diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc index 2ab4349a12cc86..7183ad2450dbe8 100644 --- a/src/main/cpp/blaze.cc +++ b/src/main/cpp/blaze.cc @@ -548,6 +548,11 @@ static vector GetServerExeArgs(const blaze_util::Path &jvm_path, } else { result.push_back("--nowindows_enable_symlinks"); } + if (startup_options.remote_repo_contents_cache) { + result.push_back("--experimental_remote_repo_contents_cache"); + // Don't set the flag to false if it's not set - non-OSS Blaze does not know + // about this flag. + } // We use this syntax so that the logic in AreStartupOptionsDifferent() that // decides whether the server needs killing is simpler. This is parsed by // the Java code where --noclient_debug and --client_debug=false are diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc index 78324c987e017d..38297f7d2dd705 100644 --- a/src/main/cpp/startup_options.cc +++ b/src/main/cpp/startup_options.cc @@ -64,7 +64,7 @@ void StartupOptions::OverrideOptionSourcesKey(const std::string &flag_name, option_sources_key_override_[flag_name] = new_name; } -StartupOptions::StartupOptions(const string &product_name, +StartupOptions::StartupOptions(const string& product_name, bool lock_install_base) : product_name(product_name), lock_install_base(lock_install_base), @@ -101,7 +101,8 @@ StartupOptions::StartupOptions(const string &product_name, cgroup_parent(), run_in_user_cgroup(false), #endif - windows_enable_symlinks(false) { + windows_enable_symlinks(false), + remote_repo_contents_cache(false) { #if defined(_WIN32) || defined(__CYGWIN__) string windows_unix_root = DetectBashAndExportBazelSh(); if (!windows_unix_root.empty()) { @@ -136,6 +137,8 @@ StartupOptions::StartupOptions(const string &product_name, RegisterNullaryStartupFlag("write_command_log", &write_command_log); RegisterNullaryStartupFlag("windows_enable_symlinks", &windows_enable_symlinks); + RegisterNullaryStartupFlag("experimental_remote_repo_contents_cache", + &remote_repo_contents_cache); #ifdef __linux__ RegisterNullaryStartupFlag("experimental_run_in_user_cgroup", &run_in_user_cgroup); diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h index 4efef10b7f9673..45c0500fb1f3a0 100644 --- a/src/main/cpp/startup_options.h +++ b/src/main/cpp/startup_options.h @@ -294,6 +294,10 @@ class StartupOptions { // developer mode to be enabled. bool windows_enable_symlinks; + // Whether to use a remote cache to store the contents of reproducible + // external repositories. + bool remote_repo_contents_cache; + protected: // Constructor for subclasses only so that site-specific extensions of this // class can override the product name. The product_name must be capitalized, diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index a2e6752c240a89..82f62b05e62f58 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -281,6 +281,16 @@ java_library( ], ) +java_library( + name = "runtime/remote_repo_contents_cache", + srcs = ["runtime/RemoteRepoContentsCache.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/vfs", + ], +) + java_library( name = "runtime", srcs = glob( @@ -301,6 +311,7 @@ java_library( "runtime/MemoryPressureEvent.java", "runtime/MemoryPressureOptions.java", "runtime/MemoryPressureStatCollector.java", + "runtime/RemoteRepoContentsCache.java", "runtime/StarlarkOptionsParser.java", "runtime/TestSummaryOptions.java", ], @@ -314,6 +325,7 @@ java_library( ":runtime/command_dispatcher", ":runtime/command_line_path_factory", ":runtime/memory_pressure", + ":runtime/remote_repo_contents_cache", ":runtime/test_summary_options", ":starlark_options_parser", "//src/main/java/com/google/devtools/build/lib/actions", diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java index 3a9812aa907d7f..abea5d93d9c361 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java @@ -153,6 +153,9 @@ private static FileStateValue createRegularFileStateValueFromPath( throws InconsistentFilesystemException { checkState(stat.isFile(), path); + if (stat instanceof FileStatusWithMetadata fileStatusWithMetadata) { + return new RegularFileStateValueWithMetadata(fileStatusWithMetadata.getMetadata()); + } try { // If the digest will be injected, we can skip calling getFastDigest, but we need to store a // contents proxy because if the digest is injected but is not available from the filesystem, @@ -386,6 +389,77 @@ public String prettyPrint() { } } + /** + * Implementation of {@link FileStateValue} for regular files when its metadata is backed by a + * {@link FileArtifactValue}. + */ + public static final class RegularFileStateValueWithMetadata extends FileStateValue { + private final FileArtifactValue metadata; + + @VisibleForTesting + public RegularFileStateValueWithMetadata(FileArtifactValue metadata) { + this.metadata = checkNotNull(metadata); + } + + @Override + public FileStateType getType() { + return FileStateType.REGULAR_FILE; + } + + @Override + public long getSize() { + return metadata.getSize(); + } + + @Override + @Nullable + public byte[] getDigest() { + return metadata.getDigest(); + } + + @Override + public FileContentsProxy getContentsProxy() { + return metadata.getContentsProxy(); + } + + public FileArtifactValue getMetadata() { + return metadata; + } + + @Override + public boolean equals(Object obj) { + if (obj == this) { + return true; + } + if (!(obj instanceof RegularFileStateValueWithMetadata other)) { + return false; + } + return other.metadata.equals(this.metadata); + } + + @Override + public int hashCode() { + return metadata.hashCode(); + } + + @Override + public byte[] getValueFingerprint() { + Fingerprint fp = new Fingerprint().addLong(getSize()); + fp.addBytes(getDigest()); + return fp.digestAndReset(); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("metadata", metadata).toString(); + } + + @Override + public String prettyPrint() { + return String.format("regular file with size of %d and %s", getSize(), metadata); + } + } + /** Implementation of {@link FileStateValue} for special files that exist. */ @VisibleForTesting public static final class SpecialFileStateValue extends FileStateValue { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java index 83f417c9a34f7c..ad781f0a63ca87 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java @@ -141,6 +141,7 @@ public Path getWorkspace() { } /** Returns working directory of the server. */ + @Nullable public Path getWorkingDirectory() { return workspace; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/BUILD index 937b912016915c..6dc818b4c6df12 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/BUILD @@ -51,6 +51,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", "//src/main/java/com/google/devtools/build/lib/rules:repository/new_local_repository_function", "//src/main/java/com/google/devtools/build/lib/rules:repository/new_local_repository_rule", + "//src/main/java/com/google/devtools/build/lib:runtime/remote_repo_contents_cache", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", "//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 45c4e7f0abbdcb..e8d3ce7c49cd80 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -99,8 +99,9 @@ import com.google.devtools.build.lib.runtime.CommonCommandOptions; import com.google.devtools.build.lib.runtime.InfoItem; import com.google.devtools.build.lib.runtime.ProcessWrapper; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; -import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutorFactory; +import com.google.devtools.build.lib.runtime.RepositoryRemoteHelpersFactory; import com.google.devtools.build.lib.runtime.ServerBuilder; import com.google.devtools.build.lib.runtime.WorkspaceBuilder; import com.google.devtools.build.lib.server.FailureDetails.ExternalRepository; @@ -151,6 +152,7 @@ public class BazelRepositoryModule extends BlazeModule { private final ImmutableMap repositoryHandlers; private final AtomicBoolean isFetch = new AtomicBoolean(false); private final StarlarkRepositoryFunction starlarkRepositoryFunction; + private RepositoryDelegatorFunction repositoryDelegatorFunction; private final RepositoryCache repositoryCache = new RepositoryCache(); private final MutableSupplier> repoEnvironmentSupplier = new MutableSupplier<>(); @@ -247,9 +249,10 @@ public void workspaceInit( SkyframeExecutorRepositoryHelpersHolder.create( new RepositoryDirectoryDirtinessChecker())); } + builder.setRepoContentsCachePathSupplier(repositoryCache.getRepoContentsCache()::getPath); // Create the repository function everything flows through. - RepositoryDelegatorFunction repositoryDelegatorFunction = + repositoryDelegatorFunction = new RepositoryDelegatorFunction( repositoryHandlers, starlarkRepositoryFunction, @@ -397,9 +400,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { if (repoContentsCachePath != null && env.getWorkspace() != null && repoContentsCachePath.startsWith(env.getWorkspace())) { - // Having the repo contents cache inside the workspace is very dangerous. During the - // lifetime of a Bazel invocation, we treat files inside the workspace as immutable. This - // can cause mysterious failures if we write files inside the workspace during the + // Having the repo contents cache inside the main repo is very dangerous. During the + // lifetime of a Bazel invocation, we treat files inside the main repo as immutable. This + // can cause mysterious failures if we write files inside the main repo during the // invocation, as is often the case with the repo contents cache. // TODO: wyv@ - This is a crude check that disables some use cases (such as when the output // base itself is inside the main repo). Investigate a better check. @@ -407,9 +410,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { throw new AbruptExitException( detailedExitCode( """ - The repo contents cache [%s] is inside the workspace [%s]. This can cause spurious \ + The repo contents cache [%s] is inside the main repo [%s]. This can cause spurious \ failures. Disable the repo contents cache with `--repo_contents_cache=`, or \ - specify `--repo_contents_cache=`. + specify `--repo_contents_cache=`. """ .formatted(repoContentsCachePath, env.getWorkspace()), Code.BAD_REPO_CONTENTS_CACHE)); @@ -425,14 +428,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { "could not acquire lock on repo contents cache", Code.BAD_REPO_CONTENTS_CACHE), e); } - if (!repoOptions.repoContentsCacheGcMaxAge.isZero()) { - env.addIdleTask( - repositoryCache - .getRepoContentsCache() - .createGcIdleTask( - repoOptions.repoContentsCacheGcMaxAge, - repoOptions.repoContentsCacheGcIdleDelay)); - } + env.addIdleTask( + repositoryCache + .getRepoContentsCache() + .createGcIdleTask( + repoOptions.repoContentsCacheGcMaxAge, + repoOptions.repoContentsCacheGcIdleDelay)); } try { @@ -657,13 +658,16 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { Optional.of(RootedPath.toRootedPath(Root.absoluteRoot(filesystem), resolvedFile)); } - RepositoryRemoteExecutorFactory remoteExecutorFactory = - env.getRuntime().getRepositoryRemoteExecutorFactory(); + RepositoryRemoteHelpersFactory repositoryRemoteHelpersFactory = + env.getRuntime().getRepositoryHelpersFactory(); RepositoryRemoteExecutor remoteExecutor = null; - if (remoteExecutorFactory != null) { - remoteExecutor = remoteExecutorFactory.create(); + RemoteRepoContentsCache remoteRepoContentsCache = null; + if (repositoryRemoteHelpersFactory != null) { + remoteExecutor = repositoryRemoteHelpersFactory.createExecutor(); + remoteRepoContentsCache = repositoryRemoteHelpersFactory.createRepoContentsCache(); } starlarkRepositoryFunction.setRepositoryRemoteExecutor(remoteExecutor); + repositoryDelegatorFunction.setRemoteRepoContentsCache(remoteRepoContentsCache); singleExtensionEvalFunction.setRepositoryRemoteExecutor(remoteExecutor); clock = env.getClock(); @@ -716,10 +720,18 @@ private String getAbsolutePath(String path, CommandEnvironment env) { */ @Nullable private Path toPath(PathFragment path, CommandEnvironment env) { - if (path.isEmpty() || env.getBlazeWorkspace().getWorkspace() == null) { + if (path.isEmpty() || env.getDirectories().getWorkspace() == null) { return null; } - return env.getBlazeWorkspace().getWorkspace().getRelative(path); + // It is important to use getWorkspace() here, not getWorkingDirectory(). Both Paths have the + // same underlying PathFragment, but may differ in their FileSystem if the remote repo contents + // cache is in use. getWorkspace() uses the same FileSystem as everything other than the + // workspace directory, while getWorkingDirectory() uses the workspace directory's FileSystem. + // Even though the users of the returned Path may end up writing to it, they are not expected to + // update source files within the workspace. Thus, the correct FileSystem is the one from + // getWorkspace(), which e.g. allows moves from the external directory under the output base to + // the local repo contents cache without crossing FileSystems. + return env.getDirectories().getWorkspace().getRelative(path); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index 398df8266ffe65..b96e3eaa1f2850 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -47,7 +47,7 @@ public abstract class BazelLockFileValue implements SkyValue { // https://cs.opensource.google/bazel/bazel/+/release-7.3.0:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java;l=120-127;drc=5f5355b75c7c93fba1e15f6658f308953f4baf51 // While this hack exists on 7.x, lockfile version increments should be done 2 at a time (i.e. // keep this number even). - public static final int LOCK_FILE_VERSION = 24; + public static final int LOCK_FILE_VERSION = 26; /** A valid empty lockfile. */ public static final BazelLockFileValue EMPTY_LOCKFILE = builder().build(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java index b5beb7880a0ed8..d489a008588783 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import com.google.gson.Gson; import com.google.gson.TypeAdapter; import com.google.gson.TypeAdapterFactory; @@ -34,6 +35,8 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; import java.util.function.Function; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; @@ -71,6 +74,14 @@ private DelegateTypeAdapterFactory( raw -> new LinkedHashMap<>((Map) raw), delegate -> ImmutableMap.copyOf((Map) delegate)); + public static final TypeAdapterFactory IMMUTABLE_SORTED_MAP = + new DelegateTypeAdapterFactory<>( + ImmutableSortedMap.class, + SortedMap.class, + TreeMap.class, + raw -> new TreeMap<>((SortedMap) raw), + delegate -> ImmutableSortedMap.copyOf((SortedMap) delegate)); + public static final TypeAdapterFactory IMMUTABLE_BIMAP = new DelegateTypeAdapterFactory<>( ImmutableBiMap.class, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index 669fda492d471d..5975763514cf68 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -19,6 +19,7 @@ import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_LIST; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_MAP; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SET; +import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SORTED_MAP; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; @@ -356,49 +357,22 @@ public ImmutableTable read(JsonReader jsonReader) } }; - private static final TypeAdapter REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER = - new TypeAdapter<>() { - @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.File value) throws IOException { - jsonWriter.value(value.toStringInternal()); - } - - @Override - public RepoRecordedInput.File read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.File) - RepoRecordedInput.File.PARSER.parse(jsonReader.nextString()); - } - }; - - private static final TypeAdapter - REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER = - new TypeAdapter<>() { - @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.Dirents value) - throws IOException { - jsonWriter.value(value.toStringInternal()); - } - - @Override - public RepoRecordedInput.Dirents read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.Dirents) - RepoRecordedInput.Dirents.PARSER.parse(jsonReader.nextString()); - } - }; - - private static final TypeAdapter - REPO_RECORDED_INPUT_ENV_VAR_TYPE_ADAPTER = + private static final TypeAdapter + REPO_RECORDED_INPUT_WITH_VALUE_TYPE_ADAPTER = new TypeAdapter<>() { @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.EnvVar value) + public void write(JsonWriter jsonWriter, RepoRecordedInput.WithValue value) throws IOException { - jsonWriter.value(value.toStringInternal()); + jsonWriter.value(value.toString()); } @Override - public RepoRecordedInput.EnvVar read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.EnvVar) - RepoRecordedInput.EnvVar.PARSER.parse(jsonReader.nextString()); + public RepoRecordedInput.WithValue read(JsonReader jsonReader) throws IOException { + return RepoRecordedInput.WithValue.parse(jsonReader.nextString()) + .orElseGet( + () -> + new RepoRecordedInput.WithValue( + RepoRecordedInput.NeverUpToDateRepoRecordedInput.PARSE_FAILURE, "")); } }; @@ -469,6 +443,7 @@ private static GsonBuilder newGsonBuilder() { .registerTypeAdapterFactory(GenerateTypeAdapter.FACTORY) .registerTypeAdapterFactory(DICT) .registerTypeAdapterFactory(IMMUTABLE_MAP) + .registerTypeAdapterFactory(IMMUTABLE_SORTED_MAP) .registerTypeAdapterFactory(IMMUTABLE_LIST) .registerTypeAdapterFactory(IMMUTABLE_BIMAP) .registerTypeAdapterFactory(IMMUTABLE_SET) @@ -485,11 +460,8 @@ private static GsonBuilder newGsonBuilder() { .registerTypeAdapter(ModuleExtensionId.IsolationKey.class, ISOLATION_KEY_TYPE_ADAPTER) .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter()) .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER) - .registerTypeAdapter(RepoRecordedInput.File.class, REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER) - .registerTypeAdapter( - RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER) .registerTypeAdapter( - RepoRecordedInput.EnvVar.class, REPO_RECORDED_INPUT_ENV_VAR_TYPE_ADAPTER); + RepoRecordedInput.WithValue.class, REPO_RECORDED_INPUT_WITH_VALUE_TYPE_ADAPTER); } private GsonTypeAdapterUtil() {} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java index a955eb70c81bfd..733bd141d652b4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java @@ -236,9 +236,7 @@ public RunModuleExtensionResult run( attributesValue)); } return new RunModuleExtensionResult( - ImmutableMap.of(), - ImmutableMap.of(), - ImmutableMap.of(), + ImmutableList.of(), generatedRepoSpecs.buildOrThrow(), ModuleExtensionMetadata.REPRODUCIBLE, ImmutableTable.of()); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 84a12f17566ce5..80d67a342fd699 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableTable; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -44,11 +45,7 @@ public static Builder builder() { @SuppressWarnings("mutable") public abstract byte[] getUsagesDigest(); - public abstract ImmutableMap getRecordedFileInputs(); - - public abstract ImmutableMap getRecordedDirentsInputs(); - - public abstract ImmutableMap> getEnvVariables(); + public abstract ImmutableList getRecordedInputs(); public abstract ImmutableMap getGeneratedRepoSpecs(); @@ -71,14 +68,7 @@ public abstract static class Builder { public abstract Builder setUsagesDigest(byte[] digest); - public abstract Builder setRecordedFileInputs( - ImmutableMap value); - - public abstract Builder setRecordedDirentsInputs( - ImmutableMap value); - - public abstract Builder setEnvVariables( - ImmutableMap> value); + public abstract Builder setRecordedInputs(ImmutableList value); public abstract Builder setGeneratedRepoSpecs(ImmutableMap value); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java index ac6deab5150f4b..48135e06ed67e8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -33,6 +34,7 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException; +import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; @@ -316,10 +318,26 @@ private RunModuleExtensionResult runInternal( } moduleContext.markSuccessful(); env.getListener().post(ModuleExtensionEvaluationProgress.finished(extensionId)); + // Combine all recorded inputs into a single list, matching the lockfile format. + // Static env vars (from the 'environ' attribute) are included alongside dynamically + // recorded env vars (from 'getenv' calls). + var recordedInputs = new ArrayList(); + // Add env vars first (static + dynamic, deduplicating with keepLast semantics). + var envVars = + ImmutableMap.>builder() + .putAll(RepoRecordedInput.EnvVar.wrap(staticEnvVars)) + .putAll(moduleContext.getRecordedEnvVarInputs()) + .buildKeepingLast(); + envVars.forEach( + (input, value) -> recordedInputs.add(new RepoRecordedInput.WithValue(input, value.orElse(null)))); + // Add file inputs. + moduleContext.getRecordedFileInputs().forEach( + (input, value) -> recordedInputs.add(new RepoRecordedInput.WithValue(input, value))); + // Add dirents inputs. + moduleContext.getRecordedDirentsInputs().forEach( + (input, value) -> recordedInputs.add(new RepoRecordedInput.WithValue(input, value))); return new RunModuleExtensionResult( - moduleContext.getRecordedFileInputs(), - moduleContext.getRecordedDirentsInputs(), - moduleContext.getRecordedEnvVarInputs(), + ImmutableList.copyOf(recordedInputs), threadContext.createRepos(starlarkSemantics), moduleExtensionMetadata, repoMappingRecorder.recordedEntries()); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java index bb5bfb83760438..ead9787e1a618e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.bazel.bzlmod; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableTable; import com.google.devtools.build.lib.cmdline.RepositoryMapping; @@ -56,9 +57,7 @@ RunModuleExtensionResult run( /* Holds the result data from running a module extension */ record RunModuleExtensionResult( - ImmutableMap recordedFileInputs, - ImmutableMap recordedDirentsInputs, - ImmutableMap> recordedEnvVarInputs, + ImmutableList recordedInputs, ImmutableMap generatedRepoSpecs, ModuleExtensionMetadata moduleExtensionMetadata, ImmutableTable recordedRepoMappingEntries) {} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 052240aa2288cd..61f2243279a100 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -21,9 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.ImmutableTable; -import com.google.common.collect.Maps; import com.google.common.collect.Table; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.RunnableExtension.RunModuleExtensionResult; @@ -47,6 +45,7 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; import java.util.Arrays; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.Function; @@ -269,15 +268,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) // result is taken from the lockfile, we can already populate the lockfile info. This is // necessary to prevent the extension from rerunning when only the imports change. if (lockfileMode == LockfileMode.UPDATE || lockfileMode == LockfileMode.REFRESH) { - var envVariables = - ImmutableMap.>builder() - // The environment variable dependencies statically declared via the 'environ' - // attribute. - .putAll(RepoRecordedInput.EnvVar.wrap(extension.getStaticEnvVars())) - // The environment variable dependencies dynamically declared via the 'getenv' method. - .putAll(moduleExtensionResult.recordedEnvVarInputs()) - .buildKeepingLast(); - lockFileInfo = Optional.of( new LockFileModuleExtension.WithFactors( @@ -287,9 +277,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) .setUsagesDigest( SingleExtensionUsagesValue.hashForEvaluation( GsonTypeAdapterUtil.SINGLE_EXTENSION_USAGES_VALUE_GSON, usagesValue)) - .setRecordedFileInputs(moduleExtensionResult.recordedFileInputs()) - .setRecordedDirentsInputs(moduleExtensionResult.recordedDirentsInputs()) - .setEnvVariables(ImmutableSortedMap.copyOf(envVariables)) + .setRecordedInputs(moduleExtensionResult.recordedInputs()) .setGeneratedRepoSpecs(generatedRepoSpecs) .setModuleExtensionMetadata(lockfileModuleExtensionMetadata) .setRecordedRepoMappingEntries( @@ -339,16 +327,6 @@ private SingleExtensionValue tryGettingValueFromLockFile( + extensionId + "' or one of its transitive .bzl files has changed"); } - if (didRecordedInputsChange( - env, - directories, - // didRecordedInputsChange expects possibly null String values. - Maps.transformValues(lockedExtension.getEnvVariables(), v -> v.orElse(null)))) { - diffRecorder.record( - "The environment variables the extension '" - + extensionId - + "' depends on (or their values) have changed"); - } // Check extension data in lockfile is still valid, disregarding usage information that is not // relevant for the evaluation of the extension. if (!Arrays.equals( @@ -363,15 +341,11 @@ private SingleExtensionValue tryGettingValueFromLockFile( + extensionId + "' have changed"); } - if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedFileInputs())) { + Optional reason = + didRecordedInputsChange(env, directories, lockedExtension.getRecordedInputs()); + if (reason.isPresent()) { diffRecorder.record( - "One or more files the extension '" + extensionId + "' is using have changed"); - } - if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedDirentsInputs())) { - diffRecorder.record( - "One or more directory listings watched by the extension '" - + extensionId - + "' have changed"); + "an input to the extension '" + extensionId + "' changed: " + reason.get()); } } catch (DiffFoundEarlyExitException ignored) { // ignored @@ -469,17 +443,23 @@ private static boolean didRepoMappingsChange( return false; } - private static boolean didRecordedInputsChange( + private static Optional didRecordedInputsChange( Environment env, BlazeDirectories directories, - Map recordedInputs) + List recordedInputs) throws InterruptedException, NeedsSkyframeRestartException { - Optional outdated = - RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputs); - if (env.valuesMissing()) { - throw new NeedsSkyframeRestartException(); + // Check inputs in batches to prevent Skyframe cycles caused by outdated dependencies. + for (ImmutableList batch : + RepoRecordedInput.WithValue.splitIntoBatches(recordedInputs)) { + Optional outdated = RepoRecordedInput.isAnyValueOutdated(env, directories, batch); + if (env.valuesMissing()) { + throw new NeedsSkyframeRestartException(); + } + if (outdated.isPresent()) { + return outdated; + } } - return outdated.isPresent(); + return Optional.empty(); } private SingleExtensionValue createSingleExtensionValue( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java index 3e7d9a4821c133..ff12bed0335665 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/VendorManager.java @@ -18,7 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.hash.HashCode; import com.google.common.hash.Hasher; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.profiler.Profiler; @@ -89,7 +89,7 @@ public void vendorRepos(Path externalRepoRoot, ImmutableList rep actualMarkerFile = cacheRepoDir .getParentDirectory() - .getChild(cacheRepoDir.getBaseName() + RepoContentsCache.RECORDED_INPUTS_SUFFIX); + .getChild(cacheRepoDir.getBaseName() + LocalRepoContentsCache.RECORDED_INPUTS_SUFFIX); } else { actualMarkerFile = markerUnderExternal; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java index a8a73e3a7a7222..c4bb4d502d8f7b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java @@ -75,7 +75,7 @@ public class RepositoryOptions extends OptionsBase { help = """ Specifies the amount of time an entry in the repo contents cache can stay unused before \ - it's garbage collected. If set to zero, garbage collection is disabled. + it's garbage collected. If set to zero, only duplicate entries will be garbage collected. """) public Duration repoContentsCacheGcMaxAge; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/BUILD index 9ad44b9549e39e..485bc8622939c2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/BUILD @@ -17,7 +17,7 @@ java_library( name = "cache", srcs = [ "DownloadCache.java", - "RepoContentsCache.java", + "LocalRepoContentsCache.java", "RepositoryCache.java", ], deps = [ diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java similarity index 73% rename from src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepoContentsCache.java rename to src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java index 411905e861c4b0..a8587387d63002 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepoContentsCache.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java @@ -15,22 +15,27 @@ package com.google.devtools.build.lib.bazel.repository.cache; import static com.google.common.collect.ImmutableList.toImmutableList; +import static java.util.Comparator.comparingLong; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.flogger.GoogleLogger; +import com.google.common.hash.HashCode; +import com.google.common.hash.HashFunction; import com.google.devtools.build.lib.server.IdleTask; import com.google.devtools.build.lib.util.FileSystemLock; import com.google.devtools.build.lib.util.FileSystemLock.LockMode; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Symlinks; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; import java.util.UUID; import javax.annotation.Nullable; @@ -42,18 +47,15 @@ * transitive bzl digest, repo attrs, starlark semantics, etc). Each distinct predeclared inputs * hash is its own entry directory in the first layer. * - *

Inside each entry directory are pairs of directories and files {@code } - * where {@code N} is an integer. The file {@code N.recorded_inputs} contains the recorded inputs - * and their values of a cached repo, and the directory {@code N} contains the cached repo contents. - * There is also a file named {@code counter} that stores the next available {@code N} for this - * entry directory, and a file named {@code lock} to ensure exclusive access to the {@code counter} - * file. + *

Inside each entry directory are pairs of directories and files {@code }. The file {@code UUID.recorded_inputs} contains the recorded inputs and + * their values of a cached repo, and the directory {@code UUID} contains the cached repo contents. * *

On a cache hit (that is, the predeclared inputs hash matches, and recorded inputs are * up-to-date), the recorded inputs file has its mtime updated. Cached repos whose recorded inputs * file is older than {@code --repo_contents_cache_gc_max_age} are garbage collected. */ -public final class RepoContentsCache { +public final class LocalRepoContentsCache { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); public static final String RECORDED_INPUTS_SUFFIX = ".recorded_inputs"; @@ -119,17 +121,19 @@ public ImmutableList getCandidateRepos(String predeclaredInputHas Preconditions.checkState(path != null); Path entryDir = path.getRelative(predeclaredInputHash); try { + // Prefer more recently used cache entries over older ones. They're more likely to be + // up-to-date; plus, if a repo is force-fetched, we want to use the new repo instead of always + // being stuck with the old one. Since the inputs file is touched on use, we can just sort by + // mtime. This is slightly more complex than in runGc below as the files may be touched + // concurrently and we need to ensure that the equality relation is consistent. + var mtimes = new HashMap(); return entryDir.getDirectoryEntries().stream() .filter(path -> path.getBaseName().endsWith(RECORDED_INPUTS_SUFFIX)) - // Prefer newer cache entries over older ones. They're more likely to be up-to-date; plus, - // if a repo is force-fetched, we want to use the new repo instead of always being stuck - // with the old one. - // To "prefer newer cache entries", we sort the entry file names by length DESC and then - // lexicographically DESC. This approximates sorting by converting to int and then DESC, - // but is defensive against non-numerically named entries. .sorted( - Comparator.comparing((Path path) -> path.getBaseName().length()) - .thenComparing(Path::getBaseName) + Comparator.comparingLong( + (Path path) -> + mtimes.computeIfAbsent( + path, LocalRepoContentsCache::getLastModifiedTimeOrZero)) .reversed()) .map(CandidateRepo::fromRecordedInputsFile) .collect(toImmutableList()); @@ -150,17 +154,17 @@ private Path ensureTrashDir() throws IOException { /** * Moves a freshly fetched repo into the contents cache. * - * @return the repo dir in the contents cache. + * @return the new cache entry */ - public Path moveToCache( + public CandidateRepo moveToCache( Path fetchedRepoDir, Path fetchedRepoMarkerFile, String predeclaredInputHash) throws IOException, InterruptedException { Preconditions.checkState(path != null); Path entryDir = path.getRelative(predeclaredInputHash); - String counter = getNextCounterInDir(entryDir); - Path cacheRecordedInputsFile = entryDir.getChild(counter + RECORDED_INPUTS_SUFFIX); - Path cacheRepoDir = entryDir.getChild(counter); + String uniqueEntryName = UUID.randomUUID().toString(); + Path cacheRecordedInputsFile = entryDir.getChild(uniqueEntryName + RECORDED_INPUTS_SUFFIX); + Path cacheRepoDir = entryDir.getChild(uniqueEntryName); cacheRepoDir.deleteTree(); cacheRepoDir.getParentDirectory().createDirectoryAndParents(); @@ -179,28 +183,7 @@ public Path moveToCache( // Set up a symlink at the original fetched repo dir path. fetchedRepoDir.deleteTree(); FileSystemUtils.ensureSymbolicLink(fetchedRepoDir, cacheRepoDir); - return cacheRepoDir; - } - - private static String getNextCounterInDir(Path entryDir) - throws IOException, InterruptedException { - Path counterFile = entryDir.getRelative("counter"); - // This use of FileSystemLock.get is safe since the predeclared input hash is part of entryDir's - // path and in particular includes the canonical repository name. This ensures that the same - // lock file will not be acquired concurrently by multiple threads, which isn't supported. - try (var lock = FileSystemLock.get(entryDir.getRelative("lock"), LockMode.EXCLUSIVE)) { - int c = 0; - if (counterFile.exists()) { - try { - c = Integer.parseInt(FileSystemUtils.readContent(counterFile, StandardCharsets.UTF_8)); - } catch (NumberFormatException e) { - // ignored - } - } - String counter = Integer.toString(c + 1); - FileSystemUtils.writeContent(counterFile, StandardCharsets.UTF_8, counter); - return counter; - } + return new CandidateRepo(cacheRecordedInputsFile, cacheRepoDir); } public void acquireSharedLock() throws IOException, InterruptedException { @@ -217,7 +200,11 @@ public void releaseSharedLock() throws IOException { /** * Creates a garbage collection {@link IdleTask} that deletes cached repos who are last accessed - * more than {@code maxAge} ago, with an idle delay of {@code idleDelay}. + * more than {@code maxAge} ago as well as duplicated repos, with an idle delay of {@code + * idleDelay}. + * + * @param maxAge the maximum age of cached repos to keep in the cache. If zero, no repo will be + * garbage collected due to age. */ public IdleTask createGcIdleTask(Duration maxAge, Duration idleDelay) { Preconditions.checkState(path != null); @@ -248,23 +235,35 @@ public void run() { private void runGc(Duration maxAge) throws InterruptedException, IOException { path.setLastModifiedTime(Path.NOW_SENTINEL_TIME); - Instant cutoff = Instant.ofEpochMilli(path.getLastModifiedTime()).minus(maxAge); + Instant cutoff = + maxAge.isZero() + ? Instant.MIN + : Instant.ofEpochMilli(path.getLastModifiedTime()).minus(maxAge); Path trashDir = ensureTrashDir(); + HashFunction sha256 = DigestHashFunction.SHA256.getHashFunction(); for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) { if (dirent.getType() != Dirent.Type.DIRECTORY || dirent.getName().equals(TRASH_PATH)) { continue; } - for (Path recordedInputsFile : path.getChild(dirent.getName()).getDirectoryEntries()) { - if (!recordedInputsFile.getBaseName().endsWith(RECORDED_INPUTS_SUFFIX)) { - continue; - } + // Sort all recorded input files by descending mtime, so that deduplication keeps around the + // most recent entry. + var recordedInputsFiles = + path.getChild(dirent.getName()).getDirectoryEntries().stream() + .filter(file -> file.getBaseName().endsWith(RECORDED_INPUTS_SUFFIX)) + .sorted(comparingLong(LocalRepoContentsCache::getLastModifiedTimeOrZero).reversed()) + .collect(toImmutableList()); + var seen = new HashSet(); + for (Path recordedInputsFile : recordedInputsFiles) { if (Thread.interrupted()) { throw new InterruptedException(); } - if (Instant.ofEpochMilli(recordedInputsFile.getLastModifiedTime()).isBefore(cutoff)) { - // Sorry buddy, you're out. + // In addition to deleting old entries, also remove identical entries. These may be created + // when multiple Bazel servers fetch the same repo at the same time. The servers that have + // their referenced entry deleted will roll over to the next entry on the next build. + if (Instant.ofEpochMilli(recordedInputsFile.getLastModifiedTime()).isBefore(cutoff) + || !seen.add(sha256.hashBytes(FileSystemUtils.readContent(recordedInputsFile)))) { recordedInputsFile.delete(); var repoDir = CandidateRepo.fromRecordedInputsFile(recordedInputsFile).contentsDir; // Use a UUID to avoid clashes. @@ -273,4 +272,13 @@ private void runGc(Duration maxAge) throws InterruptedException, IOException { } } } + + private static long getLastModifiedTimeOrZero(Path path) { + try { + return path.getLastModifiedTime(); + } catch (IOException e) { + // If we can't read the mtime from the entry, it's broken and treated as outdated. + return 0L; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java index 65276412a1a7bf..7256dc07d73e1c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java @@ -19,7 +19,7 @@ /** * A cache directory related to repositories, containing both the {@link DownloadCache} and the - * {@link RepoContentsCache}. + * {@link LocalRepoContentsCache}. */ public class RepositoryCache { // Repository cache subdirectories @@ -27,13 +27,13 @@ public class RepositoryCache { private static final String CONTENTS_DIR = "contents"; private final DownloadCache downloadCache; - private final RepoContentsCache repoContentsCache; + private final LocalRepoContentsCache repoContentsCache; @Nullable private Path path; public RepositoryCache() { downloadCache = new DownloadCache(); - repoContentsCache = new RepoContentsCache(); + repoContentsCache = new LocalRepoContentsCache(); } public void setPath(@Nullable Path path) { @@ -51,7 +51,7 @@ public DownloadCache getDownloadCache() { return downloadCache; } - public RepoContentsCache getRepoContentsCache() { + public LocalRepoContentsCache getRepoContentsCache() { return repoContentsCache; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index 76061bc9d60753..6a9e7af5fb3af5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -34,6 +34,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/repository:repository_events", "//src/main/java/com/google/devtools/build/lib/repository:request_repository_information_event", "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 47cdb8f228c06a..c6db9d1917cfb9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -49,6 +49,7 @@ 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.remote.RemoteExternalOverlayFileSystem; import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.Dirents; @@ -270,20 +271,20 @@ protected final void registerAsyncTask(AsyncTask task) { protected abstract boolean shouldDeleteWorkingDirectoryOnClose(boolean successful); /** Returns the file digests used by this context object so far. */ - public ImmutableMap getRecordedFileInputs() { + public ImmutableSortedMap getRecordedFileInputs() { return ImmutableSortedMap.copyOf(recordedFileInputs); } - public ImmutableMap getRecordedDirentsInputs() { + public ImmutableSortedMap getRecordedDirentsInputs() { return ImmutableSortedMap.copyOf(recordedDirentsInputs); } - public ImmutableMap> getRecordedEnvVarInputs() + public ImmutableSortedMap> getRecordedEnvVarInputs() throws InterruptedException { // getEnvVarValues doesn't return null since the Skyframe dependencies have already been // established by getenv calls. return RepoRecordedInput.EnvVar.wrap( - ImmutableSortedMap.copyOf(RepositoryFunction.getEnvVarValues(env, accumulatedEnvKeys))); + RepositoryFunction.getEnvVarValues(env, accumulatedEnvKeys)); } protected void checkInOutputDirectory(String operation, StarlarkPath path) @@ -2269,6 +2270,19 @@ private StarlarkPath findCommandOnPath(String program) throws IOException { // Resolve the label given by value into a file path. protected StarlarkPath getPathFromLabel(Label label) throws EvalException, InterruptedException { RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env); + if (rootedPath == null) { + throw new NeedsSkyframeRestartException(); + } + if (!label.getRepository().isMain() + && directories.getOutputBase().getFileSystem() + instanceof RemoteExternalOverlayFileSystem remoteFs) { + try { + remoteFs.ensureMaterialized(label.getRepository(), env.getListener()); + } catch (IOException e) { + throw Starlark.errorf( + "Failed to materialize remote repo %s: %s", label.getRepository(), e.getMessage()); + } + } StarlarkPath starlarkPath = new StarlarkPath(this, rootedPath.asPath()); try { maybeWatch( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 8b987a9f825132..59b79202b21653 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -19,6 +19,7 @@ import com.github.difflib.patch.PatchFailedException; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent; @@ -141,8 +142,8 @@ protected boolean shouldDeleteWorkingDirectoryOnClose(boolean successful) { return !successful; } - public ImmutableMap getRecordedDirTreeInputs() { - return ImmutableMap.copyOf(recordedDirTreeInputs); + public ImmutableSortedMap getRecordedDirTreeInputs() { + return ImmutableSortedMap.copyOf(recordedDirTreeInputs); } @StarlarkMethod( 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 87f887adb07dd8..8260e8248571a7 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 @@ -328,9 +328,8 @@ private FetchResult 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))); + // function. The env vars from the `environ` attribute are folded into the predeclared input + // hash and should not be added as separate recorded inputs. recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs()); recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirentsInputs()); recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirTreeInputs()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index fbb942f4727662..d0787cd852913a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputPrefetcher; import com.google.devtools.build.lib.actions.ActionOutputDirectoryHelper; +import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; @@ -50,6 +51,7 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.util.TempPathGenerator; import com.google.devtools.build.lib.vfs.FileSymlinkLoopException; @@ -83,7 +85,7 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet protected final Path execRoot; protected final RemoteOutputChecker remoteOutputChecker; - private final ActionOutputDirectoryHelper outputDirectoryHelper; + @Nullable private final ActionOutputDirectoryHelper outputDirectoryHelper; /** The state of a directory tracked by {@link DirectoryTracker}, as explained below. */ enum DirectoryState { @@ -124,6 +126,9 @@ void setPermanentlyWritable(Path dir) throws IOException { } private void setWritable(Path dir, DirectoryState newState) throws IOException { + if (!dir.startsWith(execRoot)) { + return; + } AtomicReference caughtException = new AtomicReference<>(); directoryStateMap.compute( @@ -135,7 +140,11 @@ private void setWritable(Path dir, DirectoryState newState) throws IOException { return newState == DirectoryState.PERMANENTLY_WRITABLE ? newState : oldState; } try { - outputDirectoryHelper.createOutputDirectory(dir, execRoot); + if (outputDirectoryHelper != null) { + outputDirectoryHelper.createOutputDirectory(dir, execRoot); + } else { + dir.createDirectoryAndParents(); + } dir.setWritable(true); } catch (IOException e) { caughtException.set(e); @@ -203,7 +212,7 @@ protected AbstractActionInputPrefetcher( Path execRoot, TempPathGenerator tempPathGenerator, RemoteOutputChecker remoteOutputChecker, - ActionOutputDirectoryHelper outputDirectoryHelper, + @Nullable ActionOutputDirectoryHelper outputDirectoryHelper, OutputPermissions outputPermissions) { this.reporter = reporter; this.execRoot = execRoot; @@ -213,6 +222,24 @@ protected AbstractActionInputPrefetcher( this.outputPermissions = outputPermissions; } + /** + * Resolves an exec path to an absolute path. On 8.7.0, external repos are at + * output_base/external/ (sibling of execroot/), so exec paths starting with "external/" must be + * resolved relative to the output base rather than the exec root. + * + *

Absolute paths (e.g., from RemoteExternalOverlayFileSystem.prefetch) are returned as-is on + * the exec root's file system. + */ + private Path resolveExecPath(PathFragment execPath) { + if (execPath.isAbsolute()) { + return execRoot.getRelative(execPath); + } + if (execPath.startsWith(LabelConstants.EXTERNAL_REPOSITORY_LOCATION)) { + return execRoot.getParentDirectory().getParentDirectory().getRelative(execPath); + } + return execRoot.getRelative(execPath); + } + private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) throws IOException { var stat = path.statIfFound(); @@ -221,12 +248,18 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) } // If an action output is stale, Skyframe will delete it prior to action execution. However, - // this doesn't apply to spawn outputs that aren't action outputs. To avoid incorrectly reusing - // one such stale output, check for its up-to-dateness here. + // this doesn't apply to spawn outputs that aren't action outputs, or to files in external repos + // that are remote repo contents cache hits. To avoid incorrectly reusing one such stale file, + // check for its up-to-dateness here. if (stat.getSize() != metadata.getSize()) { return true; } - var contentsProxy = metadata.getContentsProxy(); + FileContentsProxy contentsProxy; + try { + contentsProxy = metadata.getContentsProxy(); + } catch (UnsupportedOperationException e) { + contentsProxy = null; + } if (contentsProxy != null && contentsProxy.equals(FileContentsProxy.create(stat))) { return false; } @@ -246,7 +279,7 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) * @param tempPath the temporary path which the input should be written to. */ protected abstract ListenableFuture doDownloadFile( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -274,11 +307,33 @@ public ListenableFuture prefetchFiles( MetadataSupplier metadataSupplier, Priority priority, Reason reason) { + return prefetchFilesInterruptibly(action, inputs, metadataSupplier, priority, reason); + } + + /** + * Fetches remotely stored action outputs and stores them under their path in the output base. + * + *

The {@code inputs} may not contain any unexpanded directories. + * + *

This method is safe to be called concurrently from spawn runners before running any local + * spawn. + * + *

This method is similar to #prefetchFiles() above, but note that {@code metadataSupplier} may + * throw {@link InterruptedException}. If it does, this method will propagate this exception in + * the returned future. + * + * @return a future that is completed once all downloads have finished. + */ + public ListenableFuture prefetchFilesInterruptibly( + @Nullable ActionExecutionMetadata action, + Iterable inputs, + MetadataSupplier metadataSupplier, + Priority priority, + Reason reason) { List files = new ArrayList<>(); for (ActionInput input : inputs) { - // Source artifacts don't need to be fetched. - if (input instanceof Artifact && ((Artifact) input).isSourceArtifact()) { + if (!RemoteOutputChecker.mayBeRemote(input)) { continue; } @@ -335,7 +390,7 @@ public ListenableFuture prefetchFiles( } private ListenableFuture prefetchFile( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Set dirsWithOutputPermissions, MetadataSupplier metadataSupplier, ActionInput input, @@ -350,7 +405,7 @@ private ListenableFuture prefetchFile( PathFragment execPath = input.getExecPath(); FileArtifactValue metadata = metadataSupplier.getMetadata(input); - if (metadata == null || !canDownloadFile(execRoot.getRelative(execPath), metadata)) { + if (metadata == null || !canDownloadFile(resolveExecPath(execPath), metadata)) { return immediateVoidFuture(); } @@ -367,8 +422,8 @@ private ListenableFuture prefetchFile( Completable result = downloadFileNoCheckRx( action, - execRoot.getRelative(execPath), - treeRootExecPath != null ? execRoot.getRelative(treeRootExecPath) : null, + resolveExecPath(execPath), + treeRootExecPath != null ? resolveExecPath(treeRootExecPath) : null, dirsWithOutputPermissions, input, metadata, @@ -496,7 +551,7 @@ private Path maybeResolveSymlink(Path path) throws IOException { } private Completable downloadFileNoCheckRx( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Path path, @Nullable Path treeRoot, Set dirsWithOutputPermissions, @@ -536,7 +591,17 @@ private Completable downloadFileNoCheckRx( } Path finalPath = path; - PathFragment execPath = finalPath.relativeTo(execRoot); + PathFragment execPath; + if (finalPath.asFragment().startsWith(execRoot.asFragment())) { + execPath = finalPath.asFragment().relativeTo(execRoot.asFragment()); + } else { + // On 8.7.0, external repos are at output_base/external/ which is not under execRoot + // (output_base/execroot/_main/). Use the path relative to the output base parent instead. + execPath = + finalPath + .asFragment() + .relativeTo(execRoot.asFragment().getParentDirectory().getParentDirectory()); + } Completable download = usingTempPath( @@ -581,26 +646,30 @@ private void finalizeDownload( throws IOException { Path parentDir = checkNotNull(finalPath.getParentDirectory()); - // Ensure the parent directory exists and is writable. We cannot rely on this precondition to be - // have been established by the execution of the owning action in a previous invocation, since - // the output tree may have been externally modified in between invocations. - if (dirsWithOutputPermissions.contains(parentDir)) { - // The file belongs to a tree artifact created by an action that declared an output directory - // (as opposed to an action template expansion). The output permissions should be set on the - // parent directory after prefetching. - directoryTracker.setTemporarilyWritable(parentDir); + // Compare as fragments since execRoot may be located on a file system overlaying the host + // file system where the download is written to. + if (finalPath.asFragment().startsWith(execRoot.asFragment())) { + // Ensure the parent directory exists and is writable. We cannot rely on this precondition to + // have been established by the execution of the owning action in a previous invocation, since + // the output tree may have been externally modified in between invocations. + if (dirsWithOutputPermissions.contains(parentDir)) { + // The file belongs to a tree artifact created by an action that declared an output + // directory (as opposed to an action template expansion). The output permissions should be + // set on the parent directory after prefetching. + directoryTracker.setTemporarilyWritable(parentDir); + } else { + // One of the following must apply: + // (1) The file does not belong to a tree artifact. + // (2) The file belongs to a tree artifact created by an action template expansion. + // In case (1), the parent directory is a package or a subdirectory of a package, and should + // remain writable. In case (2), even though we arguably ought to set the output permissions + // on the parent directory to match local execution, we choose not to do it and avoid the + // additional implementation complexity required to detect a race condition between + // concurrent calls touching the same directory. + directoryTracker.setPermanentlyWritable(parentDir); + } } else { - // There are three cases: - // (1) The file does not belong to a tree artifact. - // (2) The file belongs to a tree artifact created by an action template expansion. - // (3) The file belongs to a tree artifact but we don't know it. This can occur when the - // file belongs to a tree artifact inside a fileset (see b/254844173). - // In case (1), the parent directory is a package or a subdirectory of a package, and should - // remain writable. In cases (2) and (3), even though we arguably ought to set the output - // permissions on the parent directory to match the outcome of a locally executed action, we - // choose not to do it and avoid the additional implementation complexity required to detect a - // race condition between concurrent calls touching the same directory. - directoryTracker.setPermanentlyWritable(parentDir); + parentDir.createDirectoryAndParents(); } // Set output permissions on files, matching the behavior of SkyframeActionExecutor#checkOutputs @@ -647,11 +716,11 @@ private void deletePartialDownload(Path path) { private Completable plantSymlink(Symlink symlink) { return downloadCache.executeIfNot( - execRoot.getRelative(symlink.linkExecPath()), + resolveExecPath(symlink.linkExecPath()), Completable.defer( () -> { - Path link = execRoot.getRelative(symlink.linkExecPath()); - Path target = execRoot.getRelative(symlink.targetExecPath()); + Path link = resolveExecPath(symlink.linkExecPath()); + Path target = resolveExecPath(symlink.targetExecPath()); // Delete the link path if it already exists. This is the case for tree artifacts, // whose root directory is created before the action runs. link.delete(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 3d9f9fab41049c..9d84a253c3c7f6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -58,6 +58,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:build-request-options", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib:runtime/command_line_path_factory", + "//src/main/java/com/google/devtools/build/lib:runtime/remote_repo_contents_cache", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", @@ -67,6 +68,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_tree", "//src/main/java/com/google/devtools/build/lib/actions:forbidden_action_input_exception", + "//src/main/java/com/google/devtools/build/lib/actions:important_output_handler", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", @@ -81,6 +83,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/clock", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:abstract_spawn_strategy", @@ -113,6 +116,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/unsafe:string", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", @@ -237,6 +241,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:action_output_directory_helper", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 7b6bd09109a5a8..026887e5d0c26c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -149,6 +149,10 @@ public void setActionInputFetcher(RemoteActionInputFetcher actionInputFetcher) { this.actionInputFetcher = actionInputFetcher; } + public RemoteActionInputFetcher getActionInputFetcher() { + return checkNotNull(actionInputFetcher); + } + private RemoteExecutionService getRemoteExecutionService() { if (remoteExecutionService == null) { Path workingDirectory = env.getWorkingDirectory(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index f9559ac8a4ffcd..e343fd44db9f95 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -32,12 +32,14 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import javax.annotation.Nullable; /** * Stages output files that are stored remotely to the local filesystem. * - *

This is necessary when a locally executed action consumes outputs produced by a remotely - * executed action and {@code --experimental_remote_download_outputs=minimal} is specified. + *

This is used to ensure that the inputs to a local action are present, even when they are + * provided by a remote action when building without the bytes, or by an external repository when + * building with a remote repository cache enabled. */ public class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { @@ -53,7 +55,7 @@ public class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { Path execRoot, TempPathGenerator tempPathGenerator, RemoteOutputChecker remoteOutputChecker, - ActionOutputDirectoryHelper outputDirectoryHelper, + @Nullable ActionOutputDirectoryHelper outputDirectoryHelper, OutputPermissions outputPermissions) { super( reporter, @@ -84,7 +86,7 @@ protected boolean canDownloadFile(Path path, FileArtifactValue metadata) { @Override protected ListenableFuture doDownloadFile( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -113,7 +115,11 @@ protected ListenableFuture doDownloadFile( tempPath, digest, new CombinedCache.DownloadProgressReporter( - progress -> progress.postTo(reporter, action), + progress -> { + if (action != null) { + progress.postTo(reporter, action); + } + }, execPath.toString(), digest.getSizeBytes())); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 24627b4c05978d..ac4944e1e8e7e4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1828,18 +1828,18 @@ UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) return UploadManifest.create( remoteOptions, - combinedCache.getRemoteCacheCapabilities(), - digestUtil, - action.getRemotePathResolver(), - action.getActionKey(), - action.getAction(), - action.getCommand(), - outputFiles.build(), - action.getSpawnExecutionContext().getFileOutErr(), - spawnResult.exitCode(), - spawnResult.getStartTime(), - spawnResult.getWallTimeInMs()); - + combinedCache.getRemoteCacheCapabilities(), + digestUtil, + action.getRemotePathResolver(), + action.getActionKey(), + action.getAction(), + action.getCommand(), + outputFiles.build(), + action.getSpawnExecutionContext().getFileOutErr(), + spawnResult.exitCode(), + spawnResult.getStartTime(), + spawnResult.getWallTimeInMs(), + /* preserveExecutableBit= */ false); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java new file mode 100644 index 00000000000000..fa50e82d252e47 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java @@ -0,0 +1,732 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.remote; + +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.util.concurrent.Futures.immediateCancelledFuture; +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; +import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer; +import static com.google.devtools.build.lib.util.StringEncoding.unicodeToInternal; + +import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.Directory; +import build.bazel.remote.execution.v2.Tree; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; +import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.remote.common.BulkTransferException; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.remote.util.Utils; +import com.google.devtools.build.lib.skyframe.SkyFunctions; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.Dirent; +import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.SymlinkTargetType; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.skyframe.MemoizingEvaluator; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.InterruptedIOException; +import java.io.OutputStream; +import java.nio.channels.SeekableByteChannel; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.function.Consumer; +import javax.annotation.Nullable; + +/** + * A file system that overlays the native file system with a {@link RemoteExternalFileSystem} for + * the "external" directory, which contains the contents of external repositories. + * + *

Each external repository can either be materialized to the native file system or kept in + * memory in the {@link RemoteExternalFileSystem}. + */ +public final class RemoteExternalOverlayFileSystem extends FileSystem { + + @Override + public boolean isFilePathCaseSensitive() { + return nativeFs.isFilePathCaseSensitive(); + } + + private final PathFragment externalDirectory; + private final int externalDirectorySegmentCount; + private final FileSystem nativeFs; + private final RemoteExternalFileSystem externalFs; + private final ConcurrentHashMap> materializations = + new ConcurrentHashMap<>(); + // As long as a repo name appears as a key in this map, the repo contents are available in + // externalFs. + private final ConcurrentHashMap markerFileContents = new ConcurrentHashMap<>(); + private final Set reposWithLostFiles = ConcurrentHashMap.newKeySet(); + + // Per-build information that is set in beforeCommand and cleared in afterCommand. + @Nullable private CombinedCache cache; + @Nullable private AbstractActionInputPrefetcher inputPrefetcher; + @Nullable private Reporter reporter; + @Nullable private String buildRequestId; + @Nullable private String commandId; + @Nullable private MemoizingEvaluator evaluator; + @Nullable private ExecutorService materializationExecutor; + + public RemoteExternalOverlayFileSystem(PathFragment externalDirectory, FileSystem nativeFs) { + super(nativeFs.getDigestFunction()); + this.externalDirectory = externalDirectory; + this.externalDirectorySegmentCount = externalDirectory.segmentCount(); + this.nativeFs = nativeFs; + this.externalFs = new RemoteExternalFileSystem(nativeFs.getDigestFunction()); + } + + @SuppressWarnings("AllowVirtualThreads") + public void beforeCommand( + CombinedCache cache, + AbstractActionInputPrefetcher inputPrefetcher, + Reporter reporter, + String buildRequestId, + String commandId, + MemoizingEvaluator evaluator) { + checkState( + this.cache == null + && this.inputPrefetcher == null + && this.reporter == null + && this.buildRequestId == null + && this.commandId == null + && this.evaluator == null + && this.materializationExecutor == null); + this.cache = cache; + this.inputPrefetcher = inputPrefetcher; + this.reporter = reporter; + this.buildRequestId = buildRequestId; + this.commandId = commandId; + this.evaluator = evaluator; + this.materializationExecutor = + Executors.newThreadPerTaskExecutor( + Thread.ofVirtual().name("remote-repo-materialization-", 0).factory()); + } + + public void afterCommand() { + if (cache == null) { + // Not all commands cause beforeCommand to be called, but afterCommand is called + // unconditionally. + return; + } + this.cache = null; + this.inputPrefetcher = null; + this.reporter = null; + this.buildRequestId = null; + this.commandId = null; + // Materializations happen synchronously and upon request by other repo rules, so there is no + // reason to await their orderly completion in afterCommand. + materializationExecutor.shutdownNow(); + materializationExecutor = null; + // Clean up the in-memory contents of materialized repos to save memory, or those that need to + // be refetched to recover files that the remote cache has lost. This wouldn't be safe to do + // eagerly as ongoing repo rule evaluations may still refer to the in-memory content and + // refetching is not atomic. + materializations.forEach( + 1, + (repoName, materializationState) -> + materializationState.state() == Future.State.SUCCESS + || reposWithLostFiles.contains(repoName) + ? repoName + : null, + repoName -> { + try { + externalFs.getPath(externalDirectory.getChild(repoName)).deleteTree(); + } catch (IOException e) { + throw new IllegalStateException("In-memory file system is not expected to throw", e); + } + materializations.remove(repoName); + markerFileContents.remove(repoName); + }); + if (!reposWithLostFiles.isEmpty()) { + evaluator.delete( + k -> + k.functionName().equals(SkyFunctions.REPOSITORY_DIRECTORY) + && reposWithLostFiles.contains(((RepositoryName) k.argument()).getName())); + } + reposWithLostFiles.clear(); + this.evaluator = null; + } + + /** + * Injects the given remote contents, possibly prefetching some files, and returns true on + * success. + */ + public boolean injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile) + throws IOException, InterruptedException { + var childMap = + remoteContents.getChildrenList().stream() + .collect( + toImmutableMap(cache.digestUtil::compute, directory -> directory, (a, b) -> a)); + var repoDir = externalDirectory.getChild(repo.getName()); + var filesToPrefetch = new ArrayList(); + injectRecursively( + externalFs, repoDir, remoteContents.getRoot(), childMap, filesToPrefetch::add); + try { + // TODO: This prefetches a large number of small files. Investigate whether BatchReadBlobs + // would be more efficient. + prefetch(filesToPrefetch); + } catch (BulkTransferException e) { + if (e.allCausedByCacheNotFoundException()) { + // The cache has lost the .bzl files, which should be treated just like a cache miss. + externalFs.getPath(repoDir).deleteTree(); + return false; + } + throw e; + } + // Create the repo directory on disk so that readdir reflects the overlaid state of the external + // directory. + nativeFs.createDirectoryAndParents(externalDirectory.getChild(repo.getName())); + // Keep the marker file contents in memory so that it can be written out when the repo is + // materialized. This doubles as a presence marker for the in-memory repo contents. + markerFileContents.put(repo.getName(), markerFile); + return true; + } + + private static void injectRecursively( + RemoteExternalFileSystem fs, + PathFragment path, + Directory dir, + ImmutableMap childMap, + Consumer filesToPrefetch) + throws IOException { + fs.createDirectoryAndParents(path); + for (var file : dir.getFilesList()) { + var filePath = path.getRelative(unicodeToInternal(file.getName())); + if (shouldPrefetch(filePath)) { + filesToPrefetch.accept(filePath); + } + fs.injectFile( + filePath, + FileArtifactValue.RemoteFileArtifactValue.create( + DigestUtil.toBinaryDigest(file.getDigest()), + file.getDigest().getSizeBytes(), + /* locationIndex= */ 1)); + fs.getPath(filePath).setExecutable(file.getIsExecutable()); + // The RE API does not track whether a file is readable or writable. We choose to make all + // files readable and not writable to ensure that other repo rules can't accidentally modify + // the cached repo. + fs.getPath(filePath).setWritable(false); + } + for (var symlink : dir.getSymlinksList()) { + fs.getPath(path.getRelative(unicodeToInternal(symlink.getName()))) + .createSymbolicLink(PathFragment.create(unicodeToInternal(symlink.getTarget()))); + } + for (var subdirNode : dir.getDirectoriesList()) { + var subdirPath = path.getRelative(unicodeToInternal(subdirNode.getName())); + var subdir = childMap.get(subdirNode.getDigest()); + if (subdir == null) { + throw new IOException( + "Directory %s with digest %s not found in tree" + .formatted(subdirPath, subdirNode.getDigest().getHash())); + } + injectRecursively(fs, subdirPath, subdir, childMap, filesToPrefetch); + } + } + + /** + * Materializes the given external repository to the native file system if it hasn't been + * materialized yet. This method blocks until the materialization is complete. + * + *

This should only be used for cases in which the given repo is accessed non-hermetically, + * such as when another repo rule that depends on its files executes a command. Selective reads by + * Bazel or local actions are handled automatically by the file system or {@link + * AbstractActionInputPrefetcher}. + */ + public void ensureMaterialized(RepositoryName repo, ExtendedEventHandler reporter) + throws IOException, InterruptedException { + if (!markerFileContents.containsKey(repo.getName())) { + // The repo has not been injected into the in-memory file system. + return; + } + var unused = + getFromFuture( + materializations.computeIfAbsent( + repo.getName(), + unusedRepoName -> + materializationExecutor.submit( + () -> { + doMaterialize(repo, reporter); + return null; + }))); + } + + private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) + throws IOException, InterruptedException { + reporter.handle(Event.debug("Materializing remote repo %s".formatted(repo))); + var repoPath = externalDirectory.getChild(repo.getName()); + var remoteRepo = externalFs.getPath(repoPath); + var walkResult = walk(remoteRepo); + for (var directory : walkResult.directories()) { + nativeFs.getPath(directory).createDirectory(); + } + prefetch(walkResult.files()); + // Create symlinks last as some platforms don't allow creating a symlink to a non-existent + // target. A symlink may have already been created as an input to an action. + for (var remoteSymlink : walkResult.symlinks()) { + var nativeSymlink = nativeFs.getPath(remoteSymlink); + FileSystemUtils.ensureSymbolicLink( + nativeSymlink, externalFs.getPath(remoteSymlink).readSymbolicLink()); + } + + // After the repo has been copied, atomically materialize the marker file. This ensures that the + // repo doesn't have to be refetched after the next server restart. + var markerFile = nativeFs.getPath(externalDirectory.getChild(repo.getMarkerFileName())); + var markerFileSibling = + nativeFs.getPath(externalDirectory.getChild(repo.getMarkerFileName() + ".tmp")); + FileSystemUtils.writeContentAsLatin1( + markerFileSibling, markerFileContents.remove(repo.getName())); + markerFileSibling.renameTo(markerFile); + } + + private void prefetch(List paths) throws IOException, InterruptedException { + var unused = + getFromFuture( + inputPrefetcher.prefetchFilesInterruptibly( + /* action= */ null, + Lists.transform(paths, ActionInputHelper::fromPath), + actionInput -> externalFs.getMetadata(actionInput.getExecPath()), + ActionInputPrefetcher.Priority.CRITICAL, + ActionInputPrefetcher.Reason.INPUTS)); + } + + private record WalkResult( + List files, List symlinks, List directories) {} + + private static WalkResult walk(Path root) throws IOException { + var result = new WalkResult(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); + walk(root, result); + return result; + } + + private static void walk(Path root, WalkResult result) throws IOException { + for (var dirent : root.readdir(Symlinks.NOFOLLOW)) { + var fromChild = root.getChild(dirent.getName()); + switch (dirent.getType()) { + case FILE -> result.files.add(fromChild.asFragment()); + case SYMLINK -> result.symlinks.add(fromChild.asFragment()); + case DIRECTORY -> { + result.directories.add(fromChild.asFragment()); + walk(fromChild, result); + } + default -> throw new IOException("Unsupported file type: " + dirent); + } + } + } + + /** Whether the file with the given path should be materialized eagerly when injecting a repo. */ + private static boolean shouldPrefetch(PathFragment path) { + // .bzl files are typically small and the loads between them can form complex DAGs that can only + // be discovered layer by layer, so prefetching is worthwhile to reduce the number of sequential + // cache requests. + // The REPO.bazel file, if present, is a dependency of any package and will thus have to be + // fetched anyway. + return path.getFileExtension().equals("bzl") || path.getBaseName().equals("REPO.bazel"); + } + + // Always mirror tree deletions to the underlying native file system to support bazel clean and + // repository refetching. + + @Override + protected void deleteTree(PathFragment path) throws IOException { + nativeFs.getPath(path).deleteTree(); + externalFs.getPath(path).deleteTree(); + } + + @Override + protected void deleteTreesBelow(PathFragment dir) throws IOException { + nativeFs.getPath(dir).deleteTreesBelow(); + externalFs.getPath(dir).deleteTreesBelow(); + } + + // All other methods delegate to the file system given by this method. It is important to override + // each non-final FileSystem method to benefit from optimizations implemented in the respective + // underlying file systems. + private FileSystem fsForPath(PathFragment path) { + if (path.startsWith(externalDirectory) && !path.equals(externalDirectory)) { + String repoName = path.getSegment(externalDirectorySegmentCount); + var hasBeenInjected = markerFileContents.containsKey(repoName); + var hasBeenMaterialized = + materializations.getOrDefault(repoName, immediateCancelledFuture()).state() + == Future.State.SUCCESS; + if (hasBeenInjected && !hasBeenMaterialized) { + // The repo may have been deleted due to refetching. Clean up in-memory state if that is the + // case. + if (externalFs.getPath(externalDirectory.getChild(repoName)).exists()) { + return externalFs; + } + materializations.remove(repoName); + markerFileContents.remove(repoName); + } + // Fall back to the native file system if the repo has been materialized, deleted, or never + // injected. + } + return nativeFs; + } + + /** Returns a {@link Path} on the delegate file system for the given path fragment. */ + private Path delegatePath(PathFragment path) { + return fsForPath(path).getPath(path); + } + + @Override + protected boolean delete(PathFragment path) throws IOException { + return delegatePath(path).delete(); + } + + @Override + protected byte[] getDigest(PathFragment path) throws IOException { + return delegatePath(path).getDigest(); + } + + @Nullable + @Override + protected byte[] getFastDigest(PathFragment path) throws IOException { + return delegatePath(path).getFastDigest(); + } + + @Override + public boolean supportsModifications(PathFragment path) { + return fsForPath(path).supportsModifications(path); + } + + @Override + public boolean supportsSymbolicLinksNatively(PathFragment path) { + return fsForPath(path).supportsSymbolicLinksNatively(path); + } + + @Override + public boolean supportsHardLinksNatively(PathFragment path) { + return fsForPath(path).supportsHardLinksNatively(path); + } + + @Override + public boolean createDirectory(PathFragment path) throws IOException { + return fsForPath(path).createDirectory(path); + } + + @Override + public void createDirectoryAndParents(PathFragment path) throws IOException { + fsForPath(path).createDirectoryAndParents(path); + } + + @Override + protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + return delegatePath(path).getFileSize(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + return delegatePath(path).getLastModifiedTime( + followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + public void setLastModifiedTime(PathFragment path, long newTime) throws IOException { + delegatePath(path).setLastModifiedTime(newTime); + } + + @Override + protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + return delegatePath(path).stat(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected void createSymbolicLink( + PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType hint) + throws IOException { + delegatePath(linkPath).createSymbolicLink(targetFragment); + } + + @Override + protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + return delegatePath(path).readSymbolicLink(); + } + + @Override + protected boolean exists(PathFragment path, boolean followSymlinks) { + return delegatePath(path).exists(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + public boolean exists(PathFragment path) { + return delegatePath(path).exists(); + } + + @Override + protected Collection getDirectoryEntries(PathFragment path) throws IOException { + return delegatePath(path).getDirectoryEntries().stream() + .map(Path::getBaseName) + .collect(ImmutableList.toImmutableList()); + } + + @Override + protected boolean isReadable(PathFragment path) throws IOException { + return delegatePath(path).isReadable(); + } + + @Override + protected void setReadable(PathFragment path, boolean readable) throws IOException { + delegatePath(path).setReadable(readable); + } + + @Override + protected boolean isWritable(PathFragment path) throws IOException { + return delegatePath(path).isWritable(); + } + + @Override + public void setWritable(PathFragment path, boolean writable) throws IOException { + delegatePath(path).setWritable(writable); + } + + @Override + protected boolean isExecutable(PathFragment path) throws IOException { + return delegatePath(path).isExecutable(); + } + + @Override + protected void setExecutable(PathFragment path, boolean executable) throws IOException { + delegatePath(path).setExecutable(executable); + } + + @Override + protected InputStream getInputStream(PathFragment path) throws IOException { + return delegatePath(path).getInputStream(); + } + + @Override + protected SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { + return delegatePath(path).createReadWriteByteChannel(); + } + + @Override + protected OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + throws IOException { + return delegatePath(path).getOutputStream(append); + } + + @Override + public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IOException { + delegatePath(sourcePath).renameTo(delegatePath(targetPath)); + } + + @Override + protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + throws IOException { + // Fall back to creating a copy if hard links aren't supported across file systems. + FileSystemUtils.copyFile(delegatePath(originalPath), delegatePath(linkPath)); + } + + @Override + public String getFileSystemType(PathFragment path) { + return fsForPath(path).getFileSystemType(path); + } + + @Override + public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) + throws IOException { + return fsForPath(path).getxattr(path, name, followSymlinks); + } + + @Override + protected Path resolveSymbolicLinks(PathFragment path) throws IOException { + // Ensure that the return value doesn't leave the overlay file system. + return getPath(delegatePath(path).resolveSymbolicLinks().asFragment()); + } + + @Nullable + @Override + protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { + return delegatePath(path).statNullable(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Nullable + @Override + protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { + return delegatePath(path).statIfFound(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected boolean isFile(PathFragment path, boolean followSymlinks) { + return delegatePath(path).isFile(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected boolean isSpecialFile(PathFragment path, boolean followSymlinks) { + return delegatePath(path).isSpecialFile(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected boolean isSymbolicLink(PathFragment path) { + return delegatePath(path).isSymbolicLink(); + } + + @Override + protected boolean isDirectory(PathFragment path, boolean followSymlinks) { + return delegatePath(path).isDirectory( + followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected PathFragment readSymbolicLinkUnchecked(PathFragment path) throws IOException { + return delegatePath(path).readSymbolicLinkUnchecked(); + } + + @Override + protected Collection readdir(PathFragment path, boolean followSymlinks) + throws IOException { + return delegatePath(path).readdir( + followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + } + + @Override + protected void chmod(PathFragment path, int mode) throws IOException { + delegatePath(path).chmod(mode); + } + + + private final class RemoteExternalFileSystem + extends RemoteActionFileSystem.RemoteInMemoryFileSystem { + + RemoteExternalFileSystem(DigestHashFunction hashFunction) { + super(hashFunction); + } + + private RemoteActionExecutionContext makeRemoteContext(PathFragment relativePath) { + String repoName = relativePath.subFragment(0, 1).getBaseName(); + var metadata = + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, repoName, /* actionMetadata= */ null); + // Files in the remote external repo that Bazel reads are worth writing through to the + // disk cache, as they are likely to be read again on future cold builds. + return RemoteActionExecutionContext.create(metadata) + .withReadCachePolicy(RemoteActionExecutionContext.CachePolicy.ANY_CACHE) + .withWriteCachePolicy(RemoteActionExecutionContext.CachePolicy.ANY_CACHE); + } + + private FileArtifactValue getMetadata(PathFragment path) throws IOException { + var info = + (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); + return info.getMetadata(); + } + + @Override + public synchronized InputStream getInputStream(PathFragment path) throws IOException { + if (shouldPrefetch(path)) { + return nativeFs.getPath(path).getInputStream(); + } + var relativePath = path.relativeTo(externalDirectory); + var info = + (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); + reporter.post( + new ExtendedEventHandler.FetchProgress() { + @Override + public String getResourceIdentifier() { + return relativePath.getPathString(); + } + + @Override + public String getProgress() { + return "(%s)".formatted(Utils.bytesCountToDisplayString(info.getSize())); + } + + @Override + public boolean isFinished() { + return false; + } + }); + var digest = DigestUtil.buildDigest(info.getMetadata().getDigest(), info.getSize()); + try { + var contentFuture = + cache.downloadBlob( + makeRemoteContext(relativePath), + path.getPathString(), + /* execPath= */ null, + digest); + waitForBulkTransfer(ImmutableList.of(contentFuture)); + return new ByteArrayInputStream(contentFuture.get()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new InterruptedIOException("interrupted while waiting for remote file transfer"); + } catch (BulkTransferException e) { + if (e.allCausedByCacheNotFoundException()) { + reposWithLostFiles.add(relativePath.getSegment(0)); + throw new IOException( + "%s/%s with digest %s is no longer available in the remote cache" + .formatted( + externalDirectory.getBaseName(), relativePath, DigestUtil.toString(digest)), + e); + } + throw e; + } catch (ExecutionException e) { + throw new IllegalStateException("waitForBulkTransfer should have thrown", e); + } finally { + reporter.post( + new ExtendedEventHandler.FetchProgress() { + @Override + public String getResourceIdentifier() { + return relativePath.getPathString(); + } + + @Override + public String getProgress() { + return ""; + } + + @Override + public boolean isFinished() { + return true; + } + }); + } + } + + @Override + public byte[] getDigest(PathFragment path) throws IOException { + var info = + (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); + return info.getMetadata().getDigest(); + } + + @Override + public synchronized byte[] getFastDigest(PathFragment path) throws IOException { + return getDigest(path); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index dd7b41779f46a7..2a38bd15613fb7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -50,6 +50,7 @@ import com.google.devtools.build.lib.buildtool.BuildRequest; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.ExecutionOptions; @@ -72,16 +73,19 @@ import com.google.devtools.build.lib.remote.logging.RemoteExecutionLog.LogEntry; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; +import com.google.devtools.build.lib.remote.options.RemoteStartupOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; +import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions; import com.google.devtools.build.lib.runtime.BlockWaitingModule; import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.CommandLinePathFactory; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; -import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutorFactory; +import com.google.devtools.build.lib.runtime.RepositoryRemoteHelpersFactory; import com.google.devtools.build.lib.runtime.ServerBuilder; import com.google.devtools.build.lib.runtime.WorkspaceBuilder; import com.google.devtools.build.lib.server.FailureDetails; @@ -99,6 +103,7 @@ import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParsingResult; @@ -130,7 +135,9 @@ public final class RemoteModule extends BlazeModule { MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); private final Set knownMissingCasDigests = Sets.newConcurrentHashSet(); + private boolean useRemoteRepoContentsCache; + @Nullable private PathFragment outputBase; @Nullable private AsynchronousMessageOutputStream rpcLogFile; @Nullable private ExecutorService executorService; @Nullable private RemoteActionContextProvider actionContextProvider; @@ -165,18 +172,40 @@ public ManagedChannel newChannel( private final BuildEventArtifactUploaderFactoryDelegate buildEventArtifactUploaderFactoryDelegate = new BuildEventArtifactUploaderFactoryDelegate(); - private final RepositoryRemoteExecutorFactoryDelegate repositoryRemoteExecutorFactoryDelegate = - new RepositoryRemoteExecutorFactoryDelegate(); + private final RepositoryRemoteHelpersFactoryDelegate repositoryRemoteHelpersFactoryDelegate = + new RepositoryRemoteHelpersFactoryDelegate(); private Downloader remoteDownloader; private CredentialModule credentialModule; + @Override + public ImmutableList> getStartupOptions() { + return ImmutableList.of(RemoteStartupOptions.class); + } + + @Override + public void globalInit(OptionsParsingResult startupOptions) { + outputBase = startupOptions.getOptions(BlazeServerStartupOptions.class).outputBase; + useRemoteRepoContentsCache = + startupOptions.getOptions(RemoteStartupOptions.class).useRemoteRepoContentsCache; + } + + @Nullable + @Override + public FileSystem getFileSystemForBuildArtifacts(FileSystem nativeFs) { + if (!useRemoteRepoContentsCache) { + return null; + } + return new RemoteExternalOverlayFileSystem( + outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION), nativeFs); + } + @Override public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builder) { builder.addBuildEventArtifactUploaderFactory( buildEventArtifactUploaderFactoryDelegate, "remote"); - builder.setRepositoryRemoteExecutorFactory(repositoryRemoteExecutorFactoryDelegate); + builder.setRepositoryHelpersFactory(repositoryRemoteHelpersFactoryDelegate); } /** Returns whether remote execution should be enabled. */ @@ -264,6 +293,32 @@ private void initHttpAndDiskCache( remoteOutputChecker, outputService, knownMissingCasDigests); + actionInputFetcher = createActionInputFetcher(combinedCache); + } + + @Nullable + private RemoteActionInputFetcher createActionInputFetcher(@Nullable CombinedCache combinedCache) { + if (combinedCache == null) { + return null; + } + var coreOptions = env.getOptions().getOptions(CoreOptions.class); + var outputPermissions = + coreOptions != null && coreOptions.experimentalWritableOutputs + ? OutputPermissions.WRITABLE + : OutputPermissions.READONLY; + return new RemoteActionInputFetcher( + env.getReporter(), + env.getBuildRequestId(), + env.getCommandId().toString(), + combinedCache, + env.getDirectories() + .getExecRoot(env.getRuntime().getRuleClassProvider().getRunfilesPrefix()), + tempPathGenerator, + remoteOutputChecker, + env.getOptions().getOptions(BuildRequestOptions.class) != null + ? env.getOutputDirectoryHelper() + : null, + outputPermissions); } @Override @@ -352,6 +407,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { FailureDetails.RemoteOptions.Code.DOWNLOADER_WITHOUT_GRPC_CACHE); } + tempPathGenerator = getTempPathGenerator(env); + if (!enableDiskCache && !enableHttpCache && !enableGrpcCache && !enableRemoteExecution) { // Quit if no remote caching or execution was enabled. actionContextProvider = @@ -668,15 +725,6 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOutputChecker, outputService, knownMissingCasDigests); - repositoryRemoteExecutorFactoryDelegate.init( - new RemoteRepositoryRemoteExecutorFactory( - remoteCache, - remoteExecutor, - digestUtil, - buildRequestId, - invocationId, - remoteOptions.remoteInstanceName, - remoteOptions.remoteAcceptCached)); } else { if (enableDiskCache) { try { @@ -705,6 +753,29 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { knownMissingCasDigests); } + actionInputFetcher = createActionInputFetcher(actionContextProvider.getCombinedCache()); + + repositoryRemoteHelpersFactoryDelegate.init( + new RepositoryRemoteHelpersFactoryImpl( + actionContextProvider.getCombinedCache(), + actionContextProvider.getRemoteExecutionClient(), + buildRequestId, + invocationId, + remoteOptions.remoteInstanceName, + remoteOptions.remoteAcceptCached, + remoteOptions.remoteUploadLocalResults, + verboseFailures)); + if (env.getDirectories().getOutputBase().getFileSystem() + instanceof RemoteExternalOverlayFileSystem remoteFs) { + remoteFs.beforeCommand( + actionContextProvider.getCombinedCache(), + actionInputFetcher, + env.getReporter(), + buildRequestId, + invocationId, + env.getSkyframeExecutor().getEvaluator()); + } + buildEventArtifactUploaderFactoryDelegate.init( new ByteStreamBuildEventArtifactUploaderFactory( executorService, @@ -900,7 +971,11 @@ public void afterCommand() { lastBuildId = Preconditions.checkNotNull(env).getCommandId().toString(); buildEventArtifactUploaderFactoryDelegate.reset(); - repositoryRemoteExecutorFactoryDelegate.reset(); + repositoryRemoteHelpersFactoryDelegate.reset(); + if (env.getDirectories().getOutputBase().getFileSystem() + instanceof RemoteExternalOverlayFileSystem remoteFs) { + remoteFs.afterCommand(); + } remoteDownloader = null; actionContextProvider = null; actionInputFetcher = null; @@ -969,7 +1044,7 @@ public void registerActionContexts( private TempPathGenerator getTempPathGenerator(CommandEnvironment env) throws AbruptExitException { - Path tempDir = env.getActionTempsDirectory().getChild("remote"); + Path tempDir = env.getOutputBase().getRelative("tmp/remote"); if (tempDir.exists()) { env.getReporter() .handle(Event.warn("Found stale downloads from previous build, deleting...")); @@ -993,41 +1068,20 @@ private TempPathGenerator getTempPathGenerator(CommandEnvironment env) } @Override - public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) - throws AbruptExitException { - Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null"); - Preconditions.checkState(tempPathGenerator == null, "tempPathGenerator must be null"); + public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { Preconditions.checkNotNull(remoteOptions, "remoteOptions must not be null"); if (actionContextProvider == null) { return; } - tempPathGenerator = getTempPathGenerator(env); - actionContextProvider.setTempPathGenerator(tempPathGenerator); - CoreOptions coreOptions = env.getOptions().getOptions(CoreOptions.class); - OutputPermissions outputPermissions = - coreOptions.experimentalWritableOutputs - ? OutputPermissions.WRITABLE - : OutputPermissions.READONLY; - if (actionContextProvider.getCombinedCache() != null) { Preconditions.checkNotNull(remoteOutputChecker, "remoteOutputChecker must not be null"); Preconditions.checkNotNull(outputService, "remoteOutputService must not be null"); + Preconditions.checkNotNull(actionInputFetcher, "actionInputFetcher must not be null"); - actionInputFetcher = - new RemoteActionInputFetcher( - env.getReporter(), - env.getBuildRequestId(), - env.getCommandId().toString(), - actionContextProvider.getCombinedCache(), - env.getExecRoot(), - tempPathGenerator, - remoteOutputChecker, - env.getOutputDirectoryHelper(), - outputPermissions); env.getEventBus().register(actionInputFetcher); builder.setActionInputPrefetcher(actionInputFetcher); actionContextProvider.setActionInputFetcher(actionInputFetcher); @@ -1118,12 +1172,12 @@ private static AbruptExitException createExitException( .build())); } - private static class RepositoryRemoteExecutorFactoryDelegate - implements RepositoryRemoteExecutorFactory { + private static class RepositoryRemoteHelpersFactoryDelegate + implements RepositoryRemoteHelpersFactory { - private volatile RepositoryRemoteExecutorFactory delegate; + private volatile RepositoryRemoteHelpersFactory delegate; - public void init(RepositoryRemoteExecutorFactory delegate) { + void init(RepositoryRemoteHelpersFactory delegate) { Preconditions.checkState(this.delegate == null); this.delegate = delegate; } @@ -1134,12 +1188,22 @@ public void reset() { @Nullable @Override - public RepositoryRemoteExecutor create() { - RepositoryRemoteExecutorFactory delegate = this.delegate; + public RepositoryRemoteExecutor createExecutor() { + RepositoryRemoteHelpersFactory delegate = this.delegate; + if (delegate == null) { + return null; + } + return delegate.createExecutor(); + } + + @Nullable + @Override + public RemoteRepoContentsCache createRepoContentsCache() { + RepositoryRemoteHelpersFactory delegate = this.delegate; if (delegate == null) { return null; } - return delegate.create(); + return delegate.createRepoContentsCache(); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java index 80283d4280f09f..56bd84abfdef88 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java @@ -192,24 +192,21 @@ private void addRunfiles(ProviderCollection buildTarget) { } var runfiles = runfilesSupport.getRunfiles(); for (Artifact runfile : runfiles.getArtifacts().toList()) { - if (runfile.isSourceArtifact()) { - continue; + if (mayBeRemote(runfile)) { + addOutputToDownload(runfile); } - addOutputToDownload(runfile); } for (var symlink : runfiles.getSymlinks().toList()) { var artifact = symlink.getArtifact(); - if (artifact.isSourceArtifact()) { - continue; + if (mayBeRemote(artifact)) { + addOutputToDownload(artifact); } - addOutputToDownload(artifact); } for (var symlink : runfiles.getRootSymlinks().toList()) { var artifact = symlink.getArtifact(); - if (artifact.isSourceArtifact()) { - continue; + if (mayBeRemote(artifact)) { + addOutputToDownload(artifact); } - addOutputToDownload(artifact); } } @@ -294,6 +291,19 @@ private boolean matchesPattern(PathFragment execPath) { return false; } + /** + * Returns whether this {@link ActionInput} could conceivably be only available remotely. + * + *

Use this as a quick check to avoid unnecessary extra work for artifacts that are definitely + * local. + */ + public static boolean mayBeRemote(ActionInput actionInput) { + return !(actionInput instanceof Artifact artifact + && artifact.isSourceArtifact() + // Source artifacts in the main repo don't need to be fetched. + && (artifact.getOwner() == null || artifact.getOwner().getRepository().isMain())); + } + /** Returns whether this {@link ActionInput} should be downloaded. */ @Override public boolean shouldDownloadOutput(ActionInput output, RemoteFileArtifactValue metadata) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java new file mode 100644 index 00000000000000..5202d50bd21d6b --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java @@ -0,0 +1,370 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.remote; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.util.concurrent.Futures.immediateFuture; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; +import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer; + +import build.bazel.remote.execution.v2.Action; +import build.bazel.remote.execution.v2.Command; +import build.bazel.remote.execution.v2.Directory; +import build.bazel.remote.execution.v2.Platform; +import build.bazel.remote.execution.v2.Tree; +import com.google.common.base.Splitter; +import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.exec.SpawnInputExpander; +import com.google.devtools.build.lib.exec.SpawnRunner; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy; +import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; +import com.google.devtools.build.lib.remote.common.RemotePathResolver; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; +import com.google.devtools.build.lib.unsafe.StringUnsafe; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.protobuf.ByteString; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.util.SortedMap; +import java.util.UUID; +import java.util.concurrent.ExecutionException; + +/** + * A cache for the contents of external repositories that is backed by an ordinary remote cache. + * + *

Upon a cache hit, the metadata of the files comprising the repository is downloaded and + * injected into a {@link RemoteExternalOverlayFileSystem}. Downloads of file contents only occur + * when Bazel needs to read a file (e.g., a BUILD or .bzl file) or if a file is an input to an + * action executed locally. This can save both time taken to execute repo rules and compute file + * digests and disk space required to store the contents of external repositories. + * + *

Repositories are cached as AC entries for a synthetic command with the predeclared input hash + * as the salt. The contents are represented as an output file for the marker file and an output + * directory for the contents. + * + *

At this point the cache only supports repository rules with no dependencies expressed at + * runtime. Verifying whether such dependencies are up to date can't be done via a single hash as + * the set of dependencies is not known ahead of time. Support for such rules would require a + * two-stage cache lookup in which the first lookup may produce multiple marker files. + */ +public final class RemoteRepoContentsCacheImpl implements RemoteRepoContentsCache { + private static final UUID GUID = UUID.fromString("f4a165a9-5557-45a7-bf25-230b6d42393a"); + private static final String MARKER_FILE_PATH = ".recorded_inputs"; + private static final String REPO_DIRECTORY_PATH = "repo_contents"; + + private static final Command COMMAND = + Command.newBuilder() + // A unique but nonsensical command that is valid on all platforms. It is never executed, + // but should pass all checks that an RE backend may apply to commands. + .addArguments(GUID.toString()) + .addOutputPaths(MARKER_FILE_PATH) + .addOutputPaths(REPO_DIRECTORY_PATH) + .addOutputFiles(MARKER_FILE_PATH) + .addOutputDirectories(REPO_DIRECTORY_PATH) + .setPlatform(Platform.getDefaultInstance()) + .build(); + private static final Directory INPUT_ROOT = Directory.getDefaultInstance(); + + private final CombinedCache cache; + private final String buildRequestId; + private final String commandId; + private final boolean acceptCached; + private final boolean uploadLocalResults; + private final boolean verboseFailures; + private final DigestUtil digestUtil; + private final Action baseAction; + + public RemoteRepoContentsCacheImpl( + CombinedCache cache, + String buildRequestId, + String commandId, + boolean acceptCached, + boolean uploadLocalResults, + boolean verboseFailures) { + this.buildRequestId = buildRequestId; + this.commandId = commandId; + this.cache = cache; + this.acceptCached = acceptCached; + this.uploadLocalResults = uploadLocalResults; + this.verboseFailures = verboseFailures; + this.digestUtil = cache.digestUtil; + this.baseAction = + Action.newBuilder() + .setCommandDigest(digestUtil.compute(COMMAND)) + .setInputRootDigest(digestUtil.compute(INPUT_ROOT)) + .setPlatform(Platform.getDefaultInstance()) + .build(); + } + + @Override + public void addToCache( + RepositoryName repoName, + Path fetchedRepoDir, + Path fetchedRepoMarkerFile, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws InterruptedException { + if (!(fetchedRepoDir.getFileSystem() instanceof RemoteExternalOverlayFileSystem)) { + return; + } + var context = buildContext(repoName); + if (!context.getWriteCachePolicy().allowRemoteCache()) { + return; + } + try { + if (FileSystemUtils.readLinesAsLatin1(fetchedRepoMarkerFile).stream() + .filter(line -> !line.isEmpty()) + .count() + != 1) { + // This cache currently only supports marker files that contain nothing but the predeclared + // inputs hash. Repo rules with dependencies expressed only at runtime would require a + // two-stage cache lookup. Among the rules that are supported are http_archive and + // git_repository without patches. + return; + } + } catch (IOException e) { + reporter.handle( + Event.warn( + "Failed to read marker file repo %s, skipping: %s" + .formatted(repoName, maybeGetStackTrace(e)))); + } + var action = buildAction(predeclaredInputHash); + var actionKey = new ActionKey(digestUtil.compute(action)); + var remotePathResolver = new RepoRemotePathResolver(fetchedRepoMarkerFile, fetchedRepoDir); + try { + // TODO: Consider uploading asynchronously. + var unused = + UploadManifest.create( + /* remoteOptions= */ null, + cache.getRemoteCacheCapabilities(), + digestUtil, + remotePathResolver, + actionKey, + action, + COMMAND, + ImmutableList.of(fetchedRepoMarkerFile, fetchedRepoDir), + /* outErr= */ null, + /* exitCode= */ 0, + /* startTime= */ Instant.now(), + /* wallTimeInMs= */ 0, + /* preserveExecutableBit= */ true) + .upload(context, cache, reporter); + } catch (ExecException | IOException e) { + reporter.handle( + Event.warn( + "Failed to upload repo contents to remote cache for repo %s: %s" + .formatted(repoName, maybeGetStackTrace(e)))); + } + } + + @Override + public boolean lookupCache( + RepositoryName repoName, + Path repoDir, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws IOException, InterruptedException { + try { + return doLookupCache(repoName, repoDir, predeclaredInputHash, reporter); + } catch (IOException e) { + throw new IOException( + "Failed to look up repo %s in the remote repo contents cache: %s" + .formatted(repoName, maybeGetStackTrace(e)), + e); + } + } + + private boolean doLookupCache( + RepositoryName repoName, + Path repoDir, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws IOException, InterruptedException { + if (!(repoDir.getFileSystem() instanceof RemoteExternalOverlayFileSystem remoteFs)) { + return false; + } + + var context = buildContext(repoName); + if (!context.getReadCachePolicy().allowRemoteCache()) { + return false; + } + var actionKey = new ActionKey(digestUtil.compute(buildAction(predeclaredInputHash))); + // The marker file is read right after and thus requested to be inlined. + var cachedActionResult = + cache.downloadActionResult( + context, actionKey, /* inlineOutErr= */ false, ImmutableSet.of(MARKER_FILE_PATH)); + if (cachedActionResult == null) { + return false; + } + + var actionResult = cachedActionResult.actionResult(); + if (actionResult.getExitCode() != 0 + || actionResult.getOutputFilesCount() != 1 + || actionResult.getOutputDirectoriesCount() != 1) { + reporter.handle( + Event.warn( + String.format( + "Unexpected action result for cached repo %s: exit code %d, %d output files, %d" + + " output directories", + repoName, + actionResult.getExitCode(), + actionResult.getOutputFilesCount(), + actionResult.getOutputDirectoriesCount()))); + return false; + } + + ListenableFuture markerFileContentFuture; + var markerFile = actionResult.getOutputFiles(0); + // Inlining is an optional feature, so we have to be prepared to download the marker file. + if (markerFile.getContents().isEmpty()) { + markerFileContentFuture = + cache.downloadBlob( + context, MARKER_FILE_PATH, /* execPath= */ null, markerFile.getDigest()); + } else { + markerFileContentFuture = immediateFuture(markerFile.getContents().toByteArray()); + } + var repoDirectory = actionResult.getOutputDirectories(0); + var repoDirectoryContentFuture = + Futures.transformAsync( + cache.downloadBlob( + context, REPO_DIRECTORY_PATH, /* execPath= */ null, repoDirectory.getTreeDigest()), + (treeBytes) -> immediateFuture(Tree.parseFrom(treeBytes)), + directExecutor()); + waitForBulkTransfer(ImmutableList.of(markerFileContentFuture, repoDirectoryContentFuture)); + String markerFileContent; + Tree repoDirectoryContent; + try { + markerFileContent = new String(markerFileContentFuture.get(), StandardCharsets.ISO_8859_1); + repoDirectoryContent = repoDirectoryContentFuture.get(); + } catch (ExecutionException e) { + throw new IllegalStateException( + "waitForBulkTransfer should have thrown: " + maybeGetStackTrace(e)); + } + var markerFileLines = + Splitter.on('\n') + .splitToStream(markerFileContent) + .filter(line -> !line.isEmpty()) + .collect(toImmutableList()); + if (markerFileLines.size() > 1) { + reporter.handle( + Event.warn( + "Marker file for repo %s has extra lines, skipping:\n%s" + .formatted( + repoName, + String.join("\n", markerFileLines.subList(1, markerFileLines.size()))))); + return false; + } + if (!markerFileLines.getFirst().equals(predeclaredInputHash)) { + reporter.handle( + Event.warn( + "Predeclared input hash mismatch for repo %s: expected %s, got %s" + .formatted(repoName, predeclaredInputHash, markerFileLines.getFirst()))); + return false; + } + + return remoteFs.injectRemoteRepo(repoName, repoDirectoryContent, markerFileContent); + } + + private RemoteActionExecutionContext buildContext(RepositoryName repoName) { + var metadata = + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, repoName.getName(), /* actionMetadata= */ null); + // Don't use the disk cache as `--repo_contents_cache` is a strictly better alternative for + // local caching. + return RemoteActionExecutionContext.create(metadata) + .withReadCachePolicy(acceptCached ? CachePolicy.REMOTE_CACHE_ONLY : CachePolicy.NO_CACHE) + .withWriteCachePolicy( + uploadLocalResults ? CachePolicy.REMOTE_CACHE_ONLY : CachePolicy.NO_CACHE); + } + + private Action buildAction(String predeclaredInputHash) { + // The predeclared input hash uniquely identifies the repo rule and all its attributes, but not + // dependencies established at runtime. We choose to embed it into the salt simply because that + // results in a constant Command message. + return baseAction.toBuilder() + .setSalt(ByteString.copyFrom(StringUnsafe.getByteArray(predeclaredInputHash))) + .build(); + } + + private String maybeGetStackTrace(Exception e) { + return verboseFailures ? Throwables.getStackTraceAsString(e) : e.getMessage(); + } + + private record RepoRemotePathResolver(Path fetchedRepoMarkerFile, Path fetchedRepoDir) + implements RemotePathResolver { + + @Override + public String localPathToOutputPath(Path path) { + // Map repo marker file and contents to fixed locations under the fake remote exec root. + if (path.equals(fetchedRepoMarkerFile)) { + return MARKER_FILE_PATH; + } + if (path.equals(fetchedRepoDir)) { + return REPO_DIRECTORY_PATH; + } + return REPO_DIRECTORY_PATH + "/" + path.relativeTo(fetchedRepoDir).getPathString(); + } + + @Override + public String localPathToOutputPath(PathFragment execPath) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public String getWorkingDirectory() { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public Path outputPathToLocalPath(String outputPath) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public PathFragment localPathToExecPath(PathFragment localPath) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public SortedMap getInputMapping( + SpawnRunner.SpawnExecutionContext context, boolean willAccessRepeatedly) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public void walkInputs( + Spawn spawn, + SpawnRunner.SpawnExecutionContext context, + SpawnInputExpander.InputVisitor visitor) { + throw new UnsupportedOperationException("Not used"); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java similarity index 52% rename from src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java rename to src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java index a67cf859bef185..2b4035128f7b98 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java @@ -14,48 +14,63 @@ package com.google.devtools.build.lib.remote; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; -import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; -import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutorFactory; +import com.google.devtools.build.lib.runtime.RepositoryRemoteHelpersFactory; +import javax.annotation.Nullable; -/** Factory for {@link RemoteRepositoryRemoteExecutor}. */ -class RemoteRepositoryRemoteExecutorFactory implements RepositoryRemoteExecutorFactory { +/** Factory for {@link RemoteRepositoryRemoteExecutor} and {@link RemoteRepoContentsCacheImpl}. */ +class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFactory { - private final RemoteExecutionCache remoteExecutionCache; - private final RemoteExecutionClient remoteExecutor; - private final DigestUtil digestUtil; + private final CombinedCache cache; + @Nullable private final RemoteExecutionClient remoteExecutor; private final String buildRequestId; private final String commandId; private final String remoteInstanceName; private final boolean acceptCached; + private final boolean uploadLocalResults; + private final boolean verboseFailures; - RemoteRepositoryRemoteExecutorFactory( - RemoteExecutionCache remoteExecutionCache, - RemoteExecutionClient remoteExecutor, - DigestUtil digestUtil, + RepositoryRemoteHelpersFactoryImpl( + CombinedCache cache, + @Nullable RemoteExecutionClient remoteExecutor, String buildRequestId, String commandId, String remoteInstanceName, - boolean acceptCached) { - this.remoteExecutionCache = remoteExecutionCache; + boolean acceptCached, + boolean uploadLocalResults, + boolean verboseFailures) { + this.cache = cache; this.remoteExecutor = remoteExecutor; - this.digestUtil = digestUtil; this.buildRequestId = buildRequestId; this.commandId = commandId; this.remoteInstanceName = remoteInstanceName; this.acceptCached = acceptCached; + this.uploadLocalResults = uploadLocalResults; + this.verboseFailures = verboseFailures; } + @Nullable @Override - public RepositoryRemoteExecutor create() { + public RepositoryRemoteExecutor createExecutor() { + if (remoteExecutor == null) { + return null; + } return new RemoteRepositoryRemoteExecutor( - remoteExecutionCache, + (RemoteExecutionCache) cache, remoteExecutor, - digestUtil, + cache.digestUtil, buildRequestId, commandId, remoteInstanceName, acceptCached); } + + @Nullable + @Override + public RemoteRepoContentsCache createRepoContentsCache() { + return new RemoteRepoContentsCacheImpl( + cache, buildRequestId, commandId, acceptCached, uploadLocalResults, verboseFailures); + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java index 2d968895ed258b..65e12131f5a71f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -97,11 +97,12 @@ public class UploadManifest { private final RemotePathResolver remotePathResolver; private final ActionResult.Builder result; private final boolean allowAbsoluteSymlinks; + private final boolean preserveExecutableBit; private final ConcurrentHashMap digestToFile = new ConcurrentHashMap<>(); private final ConcurrentHashMap digestToBlobs = new ConcurrentHashMap<>(); @Nullable private ActionKey actionKey; - private Digest stderrDigest; - private Digest stdoutDigest; + private Digest stderrDigest = null; + private Digest stdoutDigest = null; public static UploadManifest create( RemoteOptions remoteOptions, @@ -112,10 +113,11 @@ public static UploadManifest create( Action action, Command command, Collection outputFiles, - FileOutErr outErr, + @Nullable FileOutErr outErr, int exitCode, Instant startTime, - int wallTimeInMs) + int wallTimeInMs, + boolean preserveExecutableBit) throws ExecException, IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); result.setExitCode(exitCode); @@ -127,9 +129,12 @@ public static UploadManifest create( result, /* allowAbsoluteSymlinks= */ cacheCapabilities .getSymlinkAbsolutePathStrategy() - .equals(SymlinkAbsolutePathStrategy.Value.ALLOWED)); + .equals(SymlinkAbsolutePathStrategy.Value.ALLOWED), + preserveExecutableBit); manifest.addFiles(outputFiles); - manifest.setStdoutStderr(outErr); + if (outErr != null) { + manifest.setStdoutStderr(outErr); + } manifest.addAction(actionKey, action, command); if (manifest.getStderrDigest() != null) { result.setStderrDigest(manifest.getStderrDigest()); @@ -172,10 +177,25 @@ public UploadManifest( RemotePathResolver remotePathResolver, ActionResult.Builder result, boolean allowAbsoluteSymlinks) { + this( + digestUtil, + remotePathResolver, + result, + allowAbsoluteSymlinks, + /* preserveExecutableBit= */ false); + } + + public UploadManifest( + DigestUtil digestUtil, + RemotePathResolver remotePathResolver, + ActionResult.Builder result, + boolean allowAbsoluteSymlinks, + boolean preserveExecutableBit) { this.digestUtil = digestUtil; this.remotePathResolver = remotePathResolver; this.result = result; this.allowAbsoluteSymlinks = allowAbsoluteSymlinks; + this.preserveExecutableBit = preserveExecutableBit; } private void setStdoutStderr(FileOutErr outErr) throws IOException { @@ -229,7 +249,7 @@ void addFiles(Collection files) throws ExecException, IOException, Interru } if (statNoFollow.isFile() && !statNoFollow.isSpecialFile()) { Digest digest = digestUtil.compute(file, statNoFollow); - addFile(digest, file); + addFile(digest, file, statNoFollow); continue; } if (statNoFollow.isDirectory()) { @@ -251,7 +271,7 @@ void addFiles(Collection files) throws ExecException, IOException, Interru if (statFollow.isFile() && !statFollow.isSpecialFile()) { if (target.isAbsolute()) { // Symlink to file uploaded as a file. - addFile(digestUtil.compute(file, statFollow), file); + addFile(digestUtil.compute(file, statFollow), file, statNoFollow); } else { // Symlink to file uploaded as a symlink. if (target.isAbsolute()) { @@ -335,12 +355,12 @@ private void addDirectorySymbolicLink(Path file, PathFragment target) { result.addOutputSymlinks(outputSymlink); } - private void addFile(Digest digest, Path file) { + private void addFile(Digest digest, Path file, FileStatus statNoFollow) { result .addOutputFilesBuilder() .setPath(internalToUnicode(remotePathResolver.localPathToOutputPath(file))) .setDigest(digest) - .setIsExecutable(true); + .setIsExecutable(!preserveExecutableBit || (statNoFollow.getPermissions() & 0100) != 0); digestToFile.put(digest, file); } @@ -510,12 +530,13 @@ private void visitAsDirectory(Path path) { private void visitAsFile(Path path) throws IOException { Path parentPath = path.getParentDirectory(); + FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW); Digest digest = digestUtil.compute(path); FileNode node = FileNode.newBuilder() .setName(path.getBaseName()) .setDigest(digest) - .setIsExecutable(true) + .setIsExecutable(!preserveExecutableBit || (stat.getPermissions() & 0100) != 0) .build(); digestToFile.put(digest, path); dirToFiles.put(parentPath, node); diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java new file mode 100644 index 00000000000000..ef6c9558ecf19c --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java @@ -0,0 +1,40 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.remote.options; + +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionsBase; + +/** + * Additional startup options provided by the {@link + * com.google.devtools.build.lib.remote.RemoteModule}. + */ +public final class RemoteStartupOptions extends OptionsBase { + @Option( + name = "experimental_remote_repo_contents_cache", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + """ + If enabled, the remote cache will be used to store the results of reproducible repository + rules. If a repository rule needs to be evaluated and its result is already in the remote + cache, the contents of the repository will be kept in an in-memory file system and are + only downloaded when needed, either by Bazel itself or an action that runs locally. + """) + public boolean useRemoteRepoContentsCache; +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index 6a56e5768b1f6a..468d5f86b78a92 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -83,6 +83,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; import java.util.function.BiFunction; import javax.annotation.Nullable; @@ -92,23 +93,22 @@ public final class Utils { private Utils() {} /** - * Returns the result of a {@link ListenableFuture} if successful, or throws any checked {@link - * Exception} directly if it's an {@link IOException} or else wraps it in an {@link IOException}. + * Returns the result of a {@link Future} if successful, or throws any checked {@link Exception} + * directly if it's an {@link IOException} or else wraps it in an {@link IOException}. * *

Cancel the future on {@link InterruptedException} */ - public static T getFromFuture(ListenableFuture f) - throws IOException, InterruptedException { + public static T getFromFuture(Future f) throws IOException, InterruptedException { return getFromFuture(f, /* cancelOnInterrupt */ true); } /** - * Returns the result of a {@link ListenableFuture} if successful, or throws any checked {@link - * Exception} directly if it's an {@link IOException} or else wraps it in an {@link IOException}. + * Returns the result of a {@link Future} if successful, or throws any checked {@link Exception} + * directly if it's an {@link IOException} or else wraps it in an {@link IOException}. * * @param cancelOnInterrupt cancel the future on {@link InterruptedException} if {@code true}. */ - public static T getFromFuture(ListenableFuture f, boolean cancelOnInterrupt) + public static T getFromFuture(Future f, boolean cancelOnInterrupt) throws IOException, InterruptedException { try { return f.get(); @@ -598,7 +598,7 @@ public static void waitForBulkTransfer(Iterable> t try { if (interruptedException == null) { // Wait for all transfers to finish. - getFromFuture(transfer, /* cancelOnInterrupt= */ true); + var unused = getFromFuture(transfer, /* cancelOnInterrupt= */ true); } else { transfer.cancel(true); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index fa6e16bc3571a1..65e8a78f9d27ff 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -340,8 +340,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator", - "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function", - "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:environment_variable_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value", "//src/main/java/com/google/devtools/build/lib/skyframe:directory_tree_digest_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", @@ -401,6 +401,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/repository:external_package_exception", + "//src/main/java/com/google/devtools/build/lib:runtime/remote_repo_contents_cache", "//src/main/java/com/google/devtools/build/lib/repository:external_package_helper", "//src/main/java/com/google/devtools/build/lib/repository:external_rule_not_found_exception", "//src/main/java/com/google/devtools/build/lib/repository:repository_events", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 0819cd21c51648..e7ba136994cd32 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -15,13 +15,16 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.collect.ImmutableList.toImmutableList; -import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; +import static java.util.Comparator.naturalOrder; import static java.util.Objects.requireNonNull; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -29,8 +32,8 @@ import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; -import com.google.devtools.build.lib.skyframe.ClientEnvironmentValue; +import com.google.devtools.build.lib.skyframe.EnvironmentVariableValue; +import com.google.devtools.build.lib.skyframe.RepoEnvironmentFunction; import com.google.devtools.build.lib.skyframe.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.DirectoryTreeDigestValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; @@ -43,7 +46,7 @@ import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; -import java.util.Comparator; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; @@ -66,7 +69,7 @@ * input is stored as a string, with a prefix denoting its type, followed by a colon, and then the * information identifying that specific input. */ -public abstract sealed class RepoRecordedInput implements Comparable { +public abstract sealed class RepoRecordedInput { /** Represents a parser for a specific type of recorded inputs. */ public abstract static class Parser { /** @@ -83,14 +86,6 @@ public abstract static class Parser { public abstract RepoRecordedInput parse(String s); } - private static final Comparator COMPARATOR = - (o1, o2) -> - o1 == o2 - ? 0 - : Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix()) - .thenComparing(RepoRecordedInput::toStringInternal) - .compare(o1, o2); - /** * Parses a recorded input from its string representation. * @@ -114,6 +109,142 @@ public static RepoRecordedInput parse(String s) { return NeverUpToDateRepoRecordedInput.PARSE_FAILURE; } + /** A recorded input paired with its value. */ + public record WithValue(RepoRecordedInput input, @Nullable String value) { + /** Parses a {@link WithValue} from its string representation. */ + public static Optional parse(String s) { + int sChar = s.indexOf(' '); + if (sChar > 0) { + var input = RepoRecordedInput.parse(unescape(s.substring(0, sChar))); + if (!input.equals(NeverUpToDateRepoRecordedInput.PARSE_FAILURE)) { + return Optional.of(new WithValue(input, unescape(s.substring(sChar + 1)))); + } + } + return Optional.empty(); + } + + /** + * Splits the given map of recorded input values into batches of inputs that can be checked for + * up-to-dateness together without causing Skyframe cycles. Unlike the list overload, this + * method partitions inputs into unconditional and conditional groups since map iteration order + * (e.g. from ImmutableSortedMap) may not place unconditional inputs first. + */ + public static ImmutableList> splitIntoBatches( + Map recordedInputValues) { + var unconditional = new ArrayList(); + var conditional = new ArrayList(); + for (var e : recordedInputValues.entrySet()) { + var withValue = new WithValue(e.getKey(), e.getValue()); + if (e.getKey().canBeRequestedUnconditionally()) { + unconditional.add(withValue); + } else { + conditional.add(withValue); + } + } + var batches = ImmutableList.>builder(); + if (!unconditional.isEmpty()) { + batches.add(ImmutableList.copyOf(unconditional)); + } + if (!conditional.isEmpty()) { + batches.add(ImmutableList.copyOf(conditional)); + } + return batches.build(); + } + + /** + * Splits the given list of recorded input values into batches such that within each batch, all + * recorded inputs's {@link SkyKey}s can be requested together. Unconditional inputs are always + * in the first batch, even if the list doesn't have them first (e.g., when read from a marker + * file written with sorted map iteration order). + */ + public static ImmutableList> splitIntoBatches( + List recordedInputValues) { + var unconditional = new ArrayList(); + var conditional = new ArrayList(); + for (var recordedInputValue : recordedInputValues) { + if (recordedInputValue.input().canBeRequestedUnconditionally()) { + unconditional.add(recordedInputValue); + } else { + conditional.add(recordedInputValue); + } + } + var batches = ImmutableList.>builder(); + if (!unconditional.isEmpty()) { + batches.add(ImmutableList.copyOf(unconditional)); + } + if (!conditional.isEmpty()) { + batches.add(ImmutableList.copyOf(conditional)); + } + return batches.build(); + } + + /** Converts this {@link WithValue} to a string in a format compatible with {@link #parse}. */ + @Override + public String toString() { + return escape(input.toString()) + " " + escape(value); + } + + @VisibleForTesting + static String escape(String str) { + return str == null + ? "\\0" + : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); + } + + @VisibleForTesting + @Nullable + static String unescape(String str) { + if (str.equals("\\0")) { + return null; // \0 == null string + } + StringBuilder result = new StringBuilder(); + boolean escaped = false; + for (int i = 0; i < str.length(); i++) { + char c = str.charAt(i); + if (escaped) { + 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 + result.append(c); + } + escaped = false; + } else if (c == '\\') { + escaped = true; + } else { + result.append(c); + } + } + return result.toString(); + } + } + + /** + * Returns whether all values are still up-to-date for each recorded input. If Skyframe values are + * missing, the return value should be ignored; callers are responsible for checking {@code + * env.valuesMissing()} and triggering a Skyframe restart if needed. + */ + public static Optional isAnyValueOutdated( + Environment env, BlazeDirectories directories, List recordedInputValues) + throws InterruptedException { + env.getValuesAndExceptions( + recordedInputValues.stream() + .map(riv -> riv.input().getSkyKey(directories)) + .collect(toImmutableSet())); + if (env.valuesMissing()) { + return UNDECIDED; + } + for (var recordedInput : recordedInputValues) { + Optional reason = + recordedInput.input().isOutdated(env, directories, recordedInput.value()); + if (reason.isPresent()) { + return reason; + } + } + return Optional.empty(); + } + /** * Returns whether all values are still up-to-date for each recorded input. If Skyframe values are * missing, the return value should be ignored; callers are responsible for checking {@code @@ -153,11 +284,6 @@ public final String toString() { return getParser().getPrefix() + ":" + toStringInternal(); } - @Override - public int compareTo(RepoRecordedInput o) { - return COMPARATOR.compare(this, o); - } - /** * Returns the post-colon substring that identifies the specific input: for example, the {@code * MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. @@ -170,6 +296,13 @@ public int compareTo(RepoRecordedInput o) { /** Returns the {@link SkyKey} that is necessary to determine {@link #isOutdated}. */ public abstract SkyKey getSkyKey(BlazeDirectories directories); + /** + * Returns {@code true} if this recorded input can be requested unconditionally, i.e. without + * risking a Skyframe cycle. Recorded inputs for which this returns {@code false} must be checked + * in a separate batch from inputs for which this returns {@code true}. + */ + protected abstract boolean canBeRequestedUnconditionally(); + /** * Returns a human-readable reason for why the given {@code oldValue} is no longer up-to-date for * this recorded input, or an empty Optional if it is still up-to-date. This method can assume @@ -258,6 +391,11 @@ public final RootedPath getRootedPath(BlazeDirectories directories) { } return RootedPath.toRootedPath(root, path()); } + + /** Returns true if the path points into an external repository. */ + public boolean inExternalRepo() { + return repoName().isPresent() && !repoName().get().isMain(); + } } /** @@ -266,7 +404,7 @@ public final RootedPath getRootedPath(BlazeDirectories directories) { * of the input contains whether this is a file or a directory or nonexistent, and if it's a file, * the digest of its contents. */ - public static final class File extends RepoRecordedInput { + public static final class File extends RepoRecordedInput implements Comparable { public static final Parser PARSER = new Parser() { @Override @@ -312,6 +450,11 @@ public int hashCode() { return path.hashCode(); } + @Override + public int compareTo(File o) { + return path.toString().compareTo(o.path.toString()); + } + @Override public String toStringInternal() { return path.toString(); @@ -344,6 +487,13 @@ public SkyKey getSkyKey(BlazeDirectories directories) { return FileValue.key(path.getRootedPath(directories)); } + @Override + protected boolean canBeRequestedUnconditionally() { + // Requesting files in external repositories can result in cycles if the external repo now + // transitively depends on the requesting repo. + return !path.inExternalRepo(); + } + @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) @@ -365,7 +515,7 @@ public Optional isOutdated( } /** Represents the list of entries under a directory accessed during the fetch. */ - public static final class Dirents extends RepoRecordedInput { + public static final class Dirents extends RepoRecordedInput implements Comparable { public static final Parser PARSER = new Parser() { @Override @@ -406,6 +556,11 @@ public int hashCode() { return path.hashCode(); } + @Override + public int compareTo(Dirents o) { + return path.toString().compareTo(o.path.toString()); + } + @Override public String toStringInternal() { return path.toString(); @@ -421,6 +576,13 @@ public SkyKey getSkyKey(BlazeDirectories directories) { return DirectoryListingValue.key(path.getRootedPath(directories)); } + @Override + protected boolean canBeRequestedUnconditionally() { + // Requesting directories in external repositories can result in cycles if the external repo + // transitively depends on the requesting repo. + return !path.inExternalRepo(); + } + @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) @@ -456,7 +618,7 @@ public static String getDirentsMarkerValue(Path path) throws IOException { * (including adding/removing/renaming files or directories and changing file contents) will cause * it to go out of date. */ - public static final class DirTree extends RepoRecordedInput { + public static final class DirTree extends RepoRecordedInput implements Comparable { public static final Parser PARSER = new Parser() { @Override @@ -497,6 +659,11 @@ public int hashCode() { return path.hashCode(); } + @Override + public int compareTo(DirTree o) { + return path.toString().compareTo(o.path.toString()); + } + @Override public String toStringInternal() { return path.toString(); @@ -512,6 +679,13 @@ public SkyKey getSkyKey(BlazeDirectories directories) { return DirectoryTreeDigestValue.key(path.getRootedPath(directories)); } + @Override + protected boolean canBeRequestedUnconditionally() { + // Requesting directory trees in external repositories can result in cycles if the external + // repo now transitively depends on the requesting repo. + return !path.inExternalRepo(); + } + @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) @@ -529,7 +703,7 @@ public Optional isOutdated( } /** Represents an environment variable accessed during the repo fetch. */ - public static final class EnvVar extends RepoRecordedInput { + public static final class EnvVar extends RepoRecordedInput implements Comparable { public static final Parser PARSER = new Parser() { @Override @@ -545,10 +719,12 @@ public RepoRecordedInput parse(String s) { final String name; - public static ImmutableMap> wrap( + public static ImmutableSortedMap> wrap( Map> envVars) { return envVars.entrySet().stream() - .collect(toImmutableMap(e -> new EnvVar(e.getKey()), Map.Entry::getValue)); + .collect( + toImmutableSortedMap( + naturalOrder(), e -> new EnvVar(e.getKey()), Map.Entry::getValue)); } private EnvVar(String name) { @@ -571,6 +747,11 @@ public int hashCode() { return name.hashCode(); } + @Override + public int compareTo(EnvVar o) { + return name.compareTo(o.name); + } + @Override public Parser getParser() { return PARSER; @@ -583,17 +764,25 @@ public String toStringInternal() { @Override public SkyKey getSkyKey(BlazeDirectories directories) { - return ActionEnvironmentFunction.key(name); + return RepoEnvironmentFunction.key(name); + } + + @Override + protected boolean canBeRequestedUnconditionally() { + // Environment variables are static data injected into Skyframe, so there is no risk of + // cycles. + return true; } @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - String v = PrecomputedValue.REPO_ENV.get(env).get(name); - if (v == null) { - v = ((ClientEnvironmentValue) env.getValue(getSkyKey(directories))).getValue(); + var envValue = (EnvironmentVariableValue) env.getValue(getSkyKey(directories)); + if (envValue == null) { + return Optional.empty(); } + String v = envValue.value(); // Note that `oldValue` can be null if the env var was not set. if (!Objects.equals(oldValue, v)) { return Optional.of( @@ -673,6 +862,14 @@ public SkyKey getSkyKey(BlazeDirectories directories) { : RepositoryMappingValue.key(sourceRepo); } + @Override + protected boolean canBeRequestedUnconditionally() { + // Starlark can only request the mapping of the repo it is currently executing from, which + // means that the repo has already been fetched (either to execute the code or to verify the + // transitive .bzl hash). Further cycles aren't possible. + return true; + } + @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) @@ -741,6 +938,11 @@ public SkyKey getSkyKey(BlazeDirectories directories) { return PrecomputedValue.STARLARK_SEMANTICS.getKey(); } + @Override + protected boolean canBeRequestedUnconditionally() { + return true; + } + @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) { 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 a738356830acba..fe5eed9c881305 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,13 +21,17 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; +import com.google.devtools.build.lib.actions.FileStateValue; 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; import com.google.devtools.build.lib.bazel.bzlmod.VendorFileValue; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache.CandidateRepo; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache.CandidateRepo; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.Rule; @@ -41,10 +45,10 @@ import com.google.devtools.build.lib.repository.ExternalRuleNotFoundException; import com.google.devtools.build.lib.repository.RepositoryFailedEvent; import com.google.devtools.build.lib.repository.RepositoryFetchProgress; -import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.NeverUpToDateRepoRecordedInput; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.AlreadyReportedRepositoryAccessException; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.Reproducibility; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.skyframe.AlreadyReportedException; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; @@ -62,7 +66,7 @@ import java.io.IOException; import java.util.Map; import java.util.Optional; -import java.util.TreeMap; + import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -110,7 +114,8 @@ public final class RepositoryDelegatorFunction implements SkyFunction { private final ExternalPackageHelper externalPackageHelper; private final Supplier> repoEnvironmentSupplier; private final Supplier> clientEnvironmentSupplier; - private final RepoContentsCache repoContentsCache; + private final LocalRepoContentsCache repoContentsCache; + @Nullable private RemoteRepoContentsCache remoteRepoContentsCache; public RepositoryDelegatorFunction( ImmutableMap handlers, @@ -120,7 +125,7 @@ public RepositoryDelegatorFunction( Supplier> clientEnvironmentSupplier, BlazeDirectories directories, ExternalPackageHelper externalPackageHelper, - RepoContentsCache repoContentsCache) { + LocalRepoContentsCache repoContentsCache) { this.handlers = handlers; this.starlarkHandler = starlarkHandler; this.isFetch = isFetch; @@ -131,6 +136,10 @@ public RepositoryDelegatorFunction( this.repoContentsCache = repoContentsCache; } + public void setRemoteRepoContentsCache(RemoteRepoContentsCache remoteRepoContentsCache) { + this.remoteRepoContentsCache = remoteRepoContentsCache; + } + @Nullable @Override public SkyValue compute(SkyKey skyKey, Environment env) @@ -194,8 +203,23 @@ public SkyValue compute(SkyKey skyKey, Environment env) String.format("'%s' is not a repository rule", repositoryName)); } + @SuppressWarnings("unchecked") + Iterable environAttr = (Iterable) rule.getAttr("$environ"); + ImmutableSortedMap> environValues; + if (environAttr != null && environAttr.iterator().hasNext()) { + var rawEnvVarValues = + RepositoryFunction.getEnvVarValues(env, ImmutableSet.copyOf(environAttr)); + if (rawEnvVarValues == null) { + return null; + } + environValues = RepoRecordedInput.EnvVar.wrap(rawEnvVarValues); + } else { + environValues = ImmutableSortedMap.of(); + } + DigestWriter digestWriter = - new DigestWriter(directories, repositoryName, rule, starlarkSemantics); + new DigestWriter( + directories, repositoryName, rule, starlarkSemantics, environValues); boolean excludeRepoFromVendoring = true; if (VENDOR_DIRECTORY.get(env).isPresent()) { // If vendor mode is on @@ -221,31 +245,24 @@ public SkyValue compute(SkyKey skyKey, Environment env) || vendorFile.pinnedRepos().contains(repositoryName); } - String predeclaredInputHash = - DigestWriter.computePredeclaredInputHash(rule, starlarkSemantics); - if (shouldUseCachedRepos(env, handler, rule)) { // Make sure marker file is up-to-date; correctly describes the current repository state - var repoState = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env); - if (repoState == null) { - return null; - } - if (repoState instanceof DigestWriter.RepoDirectoryState.UpToDate) { + if (digestWriter.areRepositoryAndMarkerFileConsistent(env).isEmpty()) { return new RepositoryDirectoryValue.Success( repoRoot, /* isFetchingDelayed= */ false, excludeRepoFromVendoring); } + if (env.valuesMissing()) { + return null; + } - // Then check if the global repo contents cache has this. + // Then check if the local repo contents cache has this. if (repoContentsCache.isEnabled()) { - for (CandidateRepo candidate : - repoContentsCache.getCandidateRepos(predeclaredInputHash)) { - repoState = - digestWriter.areRepositoryAndMarkerFileConsistent( - handler, env, candidate.recordedInputsFile()); - if (repoState == null) { - return null; - } - if (repoState instanceof DigestWriter.RepoDirectoryState.UpToDate) { + ImmutableList candidateRepos = + repoContentsCache.getCandidateRepos(digestWriter.predeclaredInputHash); + for (CandidateRepo candidate : candidateRepos) { + if (digestWriter + .areRepositoryAndMarkerFileConsistent(env, candidate.recordedInputsFile()) + .isEmpty()) { if (setupOverride(candidate.contentsDir().asFragment(), env, repoRoot, repositoryName) == null) { return null; @@ -254,6 +271,25 @@ public SkyValue compute(SkyKey skyKey, Environment env) return new RepositoryDirectoryValue.Success( repoRoot, /* isFetchingDelayed= */ false, excludeRepoFromVendoring); } + if (env.valuesMissing()) { + return null; + } + } + } + + if (remoteRepoContentsCache != null) { + try { + if (remoteRepoContentsCache.lookupCache( + repositoryName, repoRoot, digestWriter.predeclaredInputHash, env.getListener())) { + return new RepositoryDirectoryValue.Success( + repoRoot, /* isFetchingDelayed= */ false, excludeRepoFromVendoring); + } + } catch (IOException e) { + env.getListener() + .handle( + Event.warn( + "Remote repo contents cache lookup failed for %s: %s" + .formatted(repositoryName, e.getMessage()))); } } } @@ -273,31 +309,65 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } digestWriter.writeMarkerFile(result.recordedInputValues()); - if (repoContentsCache.isEnabled() - && result.reproducible() == Reproducibility.YES - && !handler.isLocal(rule)) { - // This repo is eligible for the repo contents cache. - Path cachedRepoDir; - try { - cachedRepoDir = - repoContentsCache.moveToCache( - repoRoot, digestWriter.markerPath, predeclaredInputHash); - } catch (IOException e) { - throw new RepositoryFunctionException( - new IOException( - "error moving repo @@%s into the repo contents cache: %s" - .formatted(rule.getName(), e.getMessage()), - e), - Transience.TRANSIENT); + if (result.reproducible() == Reproducibility.YES && !handler.isLocal(rule)) { + if (remoteRepoContentsCache != null) { + remoteRepoContentsCache.addToCache( + repositoryName, + repoRoot, + digestWriter.markerPath, + digestWriter.predeclaredInputHash, + env.getListener()); } - // Don't forget to register a FileValue on the cache repo dir, so that we know to refetch - // if the cache entry gets GC'd from under us. - if (env.getValue( - FileValue.key( - RootedPath.toRootedPath( - Root.absoluteRoot(cachedRepoDir.getFileSystem()), cachedRepoDir))) - == null) { - return null; + if (repoContentsCache.isEnabled()) { + // This repo is eligible for the repo contents cache. + CandidateRepo candidateRepo; + try { + candidateRepo = + repoContentsCache.moveToCache( + repoRoot, digestWriter.markerPath, digestWriter.predeclaredInputHash); + } catch (IOException e) { + throw new RepositoryFunctionException( + new IOException( + "error moving repo @@%s into the repo contents cache: %s" + .formatted(rule.getName(), e.getMessage()), + e), + Transience.TRANSIENT); + } + // Don't forget to register a FileStateValue on the cache repo dir, so that we know to + // refetch if the cache entry gets GC'd from under us or the entire cache is deleted. + // + // Note that registering a FileValue dependency instead would lead to subtly incorrect + // behavior when the repo contents cache directory is deleted between builds: + // 1. We register a FileValue dependency on the cache entry. + // 2. Before the next build, the repo contents cache directory is deleted. + // 3. On the next build, FileSystemValueChecker invalidates the underlying + // FileStateValue, which in turn results in the FileValue and the current + // RepositoryDirectoryValue being marked as dirty. + // 4. Skyframe visits the dirty nodes bottom up to check for actual changes. In + // particular, it reevaluates FileFunction before RepositoryFetchFunction and thus + // the FileValue of the repo contents cache directory is locked in as non-existent + // before RepositoryFetchFunction can recreate it. + // 5. Any other SkyFunction that depends on the FileValue of a file in the repo (e.g. + // PackageFunction) will report that file as missing since the resolved path has a + // parent that is non-existent. + // By using FileStateValue directly, which benefits from special logic built into + // DirtinessCheckerUtils that recognizes the repo contents cache directories with + // non-UUID names and prevents locking in their value during dirtiness checking, we + // avoid 4. and thus the incorrect missing file errors in 5. + Path cachedRepoDir = candidateRepo.contentsDir(); + if (env.getValue( + FileStateValue.key( + RootedPath.toRootedPath( + Root.absoluteRoot(cachedRepoDir.getFileSystem()), cachedRepoDir))) + == null) { + return null; + } + // This is never reached: the repo dir in the repo contents cache is created under a new + // UUID-named directory and thus the FileStateValue above will always be missing from + // Skyframe. After the restart, the repo will either encounter the just created cache + // entry as a candidate or will create a new one if it got GC'd in the meantime. + throw new IllegalStateException( + "FileStateValue unexpectedly present for " + cachedRepoDir); } } return new RepositoryDirectoryValue.Success( @@ -355,16 +425,16 @@ private RepositoryDirectoryValue tryGettingValueUsingVendoredRepo( return setupOverride(vendorRepoPath.asFragment(), env, repoRoot, repositoryName); } - DigestWriter.RepoDirectoryState vendoredRepoState = - digestWriter.areRepositoryAndMarkerFileConsistent(handler, env, vendorMarker); - if (vendoredRepoState == null) { + Optional vendoredRepoOutdatedReason = + digestWriter.areRepositoryAndMarkerFileConsistent(env, vendorMarker); + if (env.valuesMissing()) { return null; } // If our repo is up-to-date, or this is an offline build (--nofetch), then the vendored repo // is used. - if (vendoredRepoState instanceof DigestWriter.RepoDirectoryState.UpToDate + if (vendoredRepoOutdatedReason.isEmpty() || (!IS_VENDOR_COMMAND.get(env).booleanValue() && !isFetch.get())) { - if (vendoredRepoState instanceof DigestWriter.RepoDirectoryState.OutOfDate(String reason)) { + if (vendoredRepoOutdatedReason.isPresent()) { env.getListener() .handle( Event.warn( @@ -373,7 +443,7 @@ private RepositoryDirectoryValue tryGettingValueUsingVendoredRepo( "Vendored repository '%s' is out-of-date (%s) and fetching is disabled." + " Run build without the '--nofetch' option or run" + " the bazel vendor command to update it", - rule.getName(), reason))); + rule.getName(), vendoredRepoOutdatedReason.get()))); } return setupOverride(vendorRepoPath.asFragment(), env, repoRoot, repositoryName); } else if (!IS_VENDOR_COMMAND.get(env).booleanValue()) { // build command & fetch enabled @@ -386,8 +456,7 @@ private RepositoryDirectoryValue tryGettingValueUsingVendoredRepo( "Vendored repository '%s' is out-of-date (%s). The up-to-date version will" + " be fetched into the external cache and used. To update the repo" + " in the vendor directory, run the bazel vendor command", - rule.getName(), - ((DigestWriter.RepoDirectoryState.OutOfDate) vendoredRepoState).reason()))); + rule.getName(), vendoredRepoOutdatedReason.get()))); } } else if (vendorFile.pinnedRepos().contains(repositoryName)) { throw new RepositoryFunctionException( @@ -674,30 +743,28 @@ static String unescape(String str) { } private static class DigestWriter { - // Input value map to force repo invalidation upon an invalid marker file. - private static final ImmutableMap PARSE_FAILURE = - ImmutableMap.of(NeverUpToDateRepoRecordedInput.PARSE_FAILURE, ""); - private final BlazeDirectories directories; - private final Path markerPath; - private final String ruleKey; + final String predeclaredInputHash; + final Path markerPath; DigestWriter( BlazeDirectories directories, RepositoryName repositoryName, Rule rule, - StarlarkSemantics starlarkSemantics) { + StarlarkSemantics starlarkSemantics, + ImmutableSortedMap> environValues) { this.directories = directories; - ruleKey = computePredeclaredInputHash(rule, starlarkSemantics); + predeclaredInputHash = + computePredeclaredInputHash(rule, starlarkSemantics, environValues); markerPath = getMarkerPath(directories, repositoryName); } void writeMarkerFile(Map recordedInputValues) throws RepositoryFunctionException { StringBuilder builder = new StringBuilder(); - builder.append(ruleKey).append("\n"); - for (Map.Entry recordedInput : - new TreeMap(recordedInputValues).entrySet()) { + builder.append(predeclaredInputHash).append("\n"); + for (Map.Entry recordedInput : + recordedInputValues.entrySet()) { String key = recordedInput.getKey().toString(); String value = recordedInput.getValue(); builder.append(escape(key)).append(" ").append(escape(value)).append("\n"); @@ -710,100 +777,100 @@ void writeMarkerFile(Map recordedInputValue } } - private sealed interface RepoDirectoryState { - record UpToDate() implements RepoDirectoryState {} - - record OutOfDate(String reason) implements RepoDirectoryState {} - } - - RepoDirectoryState areRepositoryAndMarkerFileConsistent( - RepositoryFunction handler, Environment env) + Optional areRepositoryAndMarkerFileConsistent(Environment env) throws InterruptedException, RepositoryFunctionException { - return areRepositoryAndMarkerFileConsistent(handler, env, markerPath); + return areRepositoryAndMarkerFileConsistent(env, markerPath); } /** - * Checks if the state of the repository in the file system is consistent with the rule in the - * WORKSPACE file. + * Checks if the state of the repo in the filesystem is consistent with its current definition. + * Returns {@link Optional#empty()} if they are consistent; otherwise, returns a description of + * why they are not. * - *

Returns null if a Skyframe status is needed. - * - *

We check the repository root for existence here, but we can't depend on the FileValue, - * because it's possible that we eventually create that directory in which case the FileValue - * and the state of the file system would be inconsistent. + *

This method treats a missing Skyframe dependency as if the repo is not up to date. The + * caller is responsible for checking {@code env.valuesMissing()}. */ - @Nullable - RepoDirectoryState areRepositoryAndMarkerFileConsistent( - RepositoryFunction handler, Environment env, Path markerPath) + Optional areRepositoryAndMarkerFileConsistent(Environment env, Path markerPath) throws RepositoryFunctionException, InterruptedException { if (!markerPath.exists()) { - return new RepoDirectoryState.OutOfDate("repo hasn't been fetched yet"); + return Optional.of("repo hasn't been fetched yet"); } try { String content = FileSystemUtils.readContent(markerPath, ISO_8859_1); - Map recordedInputValues = - readMarkerFile(content, Preconditions.checkNotNull(ruleKey)); - Optional outdatedReason = - handler.isAnyRecordedInputOutdated(directories, recordedInputValues, env); - if (env.valuesMissing()) { - return null; + Optional> recordedInputValues = + readMarkerFile(content, Preconditions.checkNotNull(predeclaredInputHash)); + if (recordedInputValues.isEmpty()) { + return Optional.of("Bazel version, flags, repo rule definition or attributes changed"); } - if (outdatedReason.isPresent()) { - return new RepoDirectoryState.OutOfDate(outdatedReason.get()); + // Check inputs in batches to prevent Skyframe cycles caused by outdated dependencies. + for (ImmutableList batch : + RepoRecordedInput.WithValue.splitIntoBatches(recordedInputValues.get())) { + Optional outdatedReason = + RepoRecordedInput.isAnyValueOutdated(env, directories, batch); + if (outdatedReason.isPresent()) { + return outdatedReason; + } } - return new RepoDirectoryState.UpToDate(); + return Optional.empty(); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } } - private static Map readMarkerFile( - String content, String expectedRuleKey) { + /** + * Returns a list of recorded inputs with their values parsed from the given marker file if the + * predeclared input hash matches, or {@code Optional.empty()} if the hash doesn't match or any + * error occurs during parsing. + */ + static Optional> readMarkerFile( + String content, String predeclaredInputHash) { Iterable lines = Splitter.on('\n').split(content); - @Nullable Map recordedInputValues = null; boolean firstLineVerified = false; + var recordedInputValues = ImmutableList.builder(); for (String line : lines) { if (line.isEmpty()) { continue; } if (!firstLineVerified) { - if (!line.equals(expectedRuleKey)) { + if (!line.equals(predeclaredInputHash)) { // Break early, need to reload anyway. This also detects marker file version changes // so that unknown formats are not parsed. - return ImmutableMap.of( - new NeverUpToDateRepoRecordedInput( - "Bazel version, flags, repo rule definition or attributes changed"), - ""); + return Optional.empty(); } firstLineVerified = true; - recordedInputValues = new TreeMap<>(); } else { - int sChar = line.indexOf(' '); - if (sChar > 0) { - RepoRecordedInput input = RepoRecordedInput.parse(unescape(line.substring(0, sChar))); - if (!input.equals(NeverUpToDateRepoRecordedInput.PARSE_FAILURE)) { - recordedInputValues.put(input, unescape(line.substring(sChar + 1))); - continue; - } + var inputAndValue = RepoRecordedInput.WithValue.parse(line); + if (inputAndValue.isEmpty()) { + // On parse failure, just forget everything else and mark the whole input out of date. + return Optional.empty(); } - // On parse failure, just forget everything else and mark the whole input out of date. - return PARSE_FAILURE; + recordedInputValues.add(inputAndValue.get()); } } if (!firstLineVerified) { - return PARSE_FAILURE; + return Optional.empty(); } - return Preconditions.checkNotNull(recordedInputValues); + return Optional.of(recordedInputValues.build()); } - static String computePredeclaredInputHash(Rule rule, StarlarkSemantics starlarkSemantics) { - return new Fingerprint() - .addBytes(RuleFormatter.serializeRule(rule).build().toByteArray()) - .addInt(MARKER_FILE_VERSION) - .addBytes(BuildLanguageOptions.stableFingerprint(starlarkSemantics)) - .hexDigestAndReset(); + static String computePredeclaredInputHash( + Rule rule, + StarlarkSemantics starlarkSemantics, + ImmutableSortedMap> environValues) { + Fingerprint fp = + new Fingerprint() + .addBytes(RuleFormatter.serializeRule(rule).build().toByteArray()) + .addInt(MARKER_FILE_VERSION) + .addBytes(BuildLanguageOptions.stableFingerprint(starlarkSemantics)) + .addInt(environValues.size()); + environValues.forEach( + (envVar, value) -> { + fp.addString(envVar.toString()); + fp.addNullableString(value.orElse(null)); + }); + return fp.hexDigestAndReset(); } private static Path getMarkerPath(BlazeDirectories directories, RepositoryName repo) { 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 62ea1b970e7b4b..ab9139b2364a4b 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,6 +20,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.cmdline.Label; @@ -266,7 +267,7 @@ public static RootedPath getRootedPathFromLabel(Label label, Environment env) * registering them as dependencies. */ @Nullable - public static ImmutableMap> getEnvVarValues( + public static ImmutableSortedMap> getEnvVarValues( Environment env, Set keys) throws InterruptedException { Map> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys); if (environ == null) { @@ -286,7 +287,7 @@ public static ImmutableMap> getEnvVarValues( repoEnv.put(key, Optional.of(value)); } } - return repoEnv.buildKeepingLast(); + return ImmutableSortedMap.copyOf(repoEnv.buildKeepingLast()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index 34352881c38543..addfec3cdfa029 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -195,7 +195,7 @@ public final class BlazeRuntime implements BugReport.BlazeRuntimeInterface { private final BuildEventArtifactUploaderFactoryMap buildEventArtifactUploaderFactoryMap; private final ActionKeyContext actionKeyContext; private final ImmutableMap authHeadersProviderMap; - @Nullable private final RepositoryRemoteExecutorFactory repositoryRemoteExecutorFactory; + @Nullable private final RepositoryRemoteHelpersFactory repositoryRemoteHelpersFactory; // Workspace state (currently exactly one workspace per server) private BlazeWorkspace workspace; @@ -228,7 +228,7 @@ private BlazeRuntime( String productName, BuildEventArtifactUploaderFactoryMap buildEventArtifactUploaderFactoryMap, ImmutableMap authHeadersProviderMap, - RepositoryRemoteExecutorFactory repositoryRemoteExecutorFactory, + RepositoryRemoteHelpersFactory repositoryRemoteHelpersFactory, InstrumentationOutputFactory instrumentationOutputFactory, FileSystemLock installBaseLock) { // Server state @@ -259,7 +259,7 @@ private BlazeRuntime( this.buildEventArtifactUploaderFactoryMap = buildEventArtifactUploaderFactoryMap; this.authHeadersProviderMap = Preconditions.checkNotNull(authHeadersProviderMap, "authHeadersProviderMap"); - this.repositoryRemoteExecutorFactory = repositoryRemoteExecutorFactory; + this.repositoryRemoteHelpersFactory = repositoryRemoteHelpersFactory; this.instrumentationOutputFactory = instrumentationOutputFactory; this.installBaseLock = installBaseLock; } @@ -1568,8 +1568,8 @@ public ImmutableMap getAuthHeadersProvidersMap() { return authHeadersProviderMap; } - public RepositoryRemoteExecutorFactory getRepositoryRemoteExecutorFactory() { - return repositoryRemoteExecutorFactory; + public RepositoryRemoteHelpersFactory getRepositoryHelpersFactory() { + return repositoryRemoteHelpersFactory; } /** @@ -1706,7 +1706,7 @@ public BlazeRuntime build() throws AbruptExitException { productName, serverBuilder.getBuildEventArtifactUploaderMap(), serverBuilder.getAuthHeadersProvidersMap(), - serverBuilder.getRepositoryRemoteExecutorFactory(), + serverBuilder.getRepositoryHelpersFactory(), serverBuilder.createInstrumentationOutputFactory(), installBaseLock); AutoProfiler.setClock(runtime.getClock()); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java new file mode 100644 index 00000000000000..8c4f0f25d1cf41 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java @@ -0,0 +1,45 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.runtime; + +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.vfs.Path; +import java.io.IOException; + +/** A remote cache for the contents of external repositories. */ +public interface RemoteRepoContentsCache { + /** Adds a repository that has been fetched locally to the remote cache. */ + void addToCache( + RepositoryName repoName, + Path fetchedRepoDir, + Path fetchedRepoMarkerFile, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws InterruptedException; + + /** + * Retrieves a repository from the remote cache if possible. + * + * @return true if there was a cache hit and the repository has been fetched into the given + * directory. + */ + boolean lookupCache( + RepositoryName repoName, + Path repoDir, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws IOException, InterruptedException; +} diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java similarity index 70% rename from src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteExecutorFactory.java rename to src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java index aa69457248e1e2..d3cd1e66ced683 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java @@ -15,10 +15,14 @@ import javax.annotation.Nullable; -/** Factory for {@link RepositoryRemoteExecutor}. */ -public interface RepositoryRemoteExecutorFactory { +/** Factory for {@link RepositoryRemoteExecutor} and {@link RemoteRepoContentsCache}. */ +public interface RepositoryRemoteHelpersFactory { /** Returns a new {@link RepositoryRemoteExecutor} or {@code null}. */ @Nullable - RepositoryRemoteExecutor create(); + RepositoryRemoteExecutor createExecutor(); + + /** Returns a new {@link RemoteRepoContentsCache} or {@code null}. */ + @Nullable + RemoteRepoContentsCache createRepoContentsCache(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java b/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java index bb788c471f25cd..68b8cd2a82d9e0 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java @@ -39,7 +39,7 @@ public final class ServerBuilder { new BuildEventArtifactUploaderFactoryMap.Builder(); private final ImmutableMap.Builder authHeadersProvidersMap = ImmutableMap.builder(); - private RepositoryRemoteExecutorFactory repositoryRemoteExecutorFactory; + private RepositoryRemoteHelpersFactory repositoryRemoteHelpersFactory; private final InstrumentationOutputFactory.Builder instrumentationOutputFactoryBuilder = new InstrumentationOutputFactory.Builder(); @@ -77,8 +77,8 @@ public BuildEventArtifactUploaderFactoryMap getBuildEventArtifactUploaderMap() { return buildEventArtifactUploaderFactories.build(); } - public RepositoryRemoteExecutorFactory getRepositoryRemoteExecutorFactory() { - return repositoryRemoteExecutorFactory; + public RepositoryRemoteHelpersFactory getRepositoryHelpersFactory() { + return repositoryRemoteHelpersFactory; } /** @@ -166,9 +166,9 @@ public ServerBuilder addBuildEventArtifactUploaderFactory( } @CanIgnoreReturnValue - public ServerBuilder setRepositoryRemoteExecutorFactory( - RepositoryRemoteExecutorFactory repositoryRemoteExecutorFactory) { - this.repositoryRemoteExecutorFactory = repositoryRemoteExecutorFactory; + public ServerBuilder setRepositoryHelpersFactory( + RepositoryRemoteHelpersFactory repositoryRemoteHelpersFactory) { + this.repositoryRemoteHelpersFactory = repositoryRemoteHelpersFactory; return this; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java b/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java index bc6b10eb21dad3..9c000a1e957f09 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueService; import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecRegistry; import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SingleFileSystemSyscallCache; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; @@ -66,6 +67,7 @@ public final class WorkspaceBuilder { private SyscallCache syscallCache; private boolean allowExternalRepositories = true; + private Supplier repoContentsCachePathSupplier = () -> null; @Nullable private Supplier analysisCodecRegistrySupplier = null; @Nullable private FingerprintValueService.Factory fingerprintValueServiceFactory = null; @@ -125,6 +127,7 @@ BlazeWorkspace build( skyFunctions.buildOrThrow(), singleFsSyscallCache, skyframeExecutorRepositoryHelpersHolder, + repoContentsCachePathSupplier, skyKeyStateReceiver == null ? SkyframeExecutor.SkyKeyStateReceiver.NULL_INSTANCE : skyKeyStateReceiver, @@ -228,6 +231,13 @@ public WorkspaceBuilder setAllowExternalRepositories(boolean allowExternalReposi return this; } + @CanIgnoreReturnValue + public WorkspaceBuilder setRepoContentsCachePathSupplier( + Supplier repoContentsCachePathSupplier) { + this.repoContentsCachePathSupplier = repoContentsCachePathSupplier; + return this; + } + @CanIgnoreReturnValue public WorkspaceBuilder setSkyKeyStateReceiver( SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index 77fedd0f818fa6..c1fcd687ca2ca3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -132,6 +132,18 @@ private ActionOutputMetadataStore( this.execRoot = checkNotNull(execRoot); } + /** + * Relativizes the given path against the exec root, falling back to the output base for paths + * not under the exec root (e.g., external repos on 8.7.0 which are at output_base/external/). + */ + private PathFragment relativizeToExecRootOrOutputBase(PathFragment path) { + if (path.startsWith(execRoot)) { + return path.relativeTo(execRoot); + } + // External repos on 8.7.0 are at output_base/external/, a sibling of execroot/. + return path.relativeTo(execRoot.getParentDirectory().getParentDirectory()); + } + private void putArtifactData(Artifact artifact, FileArtifactValue value) { Preconditions.checkArgument( !artifact.isTreeArtifact() && !artifact.isChildOfDeclaredDirectory(), @@ -326,7 +338,8 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa tree.setMaterializationExecPath( metadata .getMaterializationExecPath() - .orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot))); + .orElse(relativizeToExecRootOrOutputBase( + treeDir.resolveSymbolicLinks().asFragment()))); } return tree.build(); @@ -469,7 +482,7 @@ private FileArtifactValue constructFileArtifactValue( value = RemoteFileArtifactValue.createFromExistingWithMaterializationPath( (RemoteFileArtifactValue) value, - statAndValue.realPath().asFragment().relativeTo(execRoot)); + relativizeToExecRootOrOutputBase(statAndValue.realPath().asFragment())); } // Ensure that we don't have both an injected digest and a digest from the filesystem. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index 1158ed7a71e3c3..619860b2716293 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithMetadata; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot; import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode; @@ -313,6 +314,12 @@ private SkyValue createSourceValue(Artifact artifact, Environment env) if (!fileValue.exists()) { return new MissingArtifactValue(artifact); } + if (fileValue.realFileStateValue() + instanceof RegularFileStateValueWithMetadata valueWithMetadata) { + var metadata = valueWithMetadata.getMetadata(); + sourceArtifactsSeen.accumulate(metadata); + return metadata; + } if (fileValue.isDirectory()) { env.getListener().post(SourceDirectoryEvent.create(artifact.getExecPath())); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 7a741b87cbf93b..ecabd7059116d0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -110,6 +110,7 @@ java_library( ":cached_bzl_load_value_and_deps_builder_factory", ":client_environment_function", ":client_environment_value", + ":environment_variable_value", ":collect_packages_under_directory_function", ":collect_packages_under_directory_value", ":collect_targets_in_package_function", @@ -173,6 +174,7 @@ java_library( ":recursive_package_provider_backed_target_pattern_resolver", ":recursive_pkg_function", ":recursive_pkg_value", + ":repo_environment_function", ":repo_file_function", ":repo_package_args_function", ":repository_mapping_function", @@ -1025,6 +1027,16 @@ java_library( ], ) +java_library( + name = "environment_variable_value", + srcs = ["EnvironmentVariableValue.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:jsr305", + ], +) + java_library( name = "collect_packages_under_directory_function", srcs = ["CollectPackagesUnderDirectoryFunction.java"], @@ -2335,6 +2347,23 @@ java_library( ], ) +java_library( + name = "repo_environment_function", + srcs = ["RepoEnvironmentFunction.java"], + deps = [ + ":client_environment_function", + ":client_environment_value", + ":environment_variable_value", + ":precomputed_value", + ":sky_functions", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:guava", + "//third_party:jsr305", + ], +) + java_library( name = "repo_file_function", srcs = ["RepoFileFunction.java"], @@ -2532,6 +2561,7 @@ java_library( ":top_level_conflict_exception", ":transitive_target_key", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", @@ -2669,6 +2699,7 @@ java_library( ":repository_mapping_value", ":sky_functions", ":starlark_builtins_value", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution", @@ -2677,6 +2708,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/repository:request_repository_information_event", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value", + "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java index 4cc6c72ac182d6..ac846e0885fbe8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java @@ -20,6 +20,7 @@ import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue; import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleResolutionValue; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; @@ -32,6 +33,7 @@ import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.repository.RequestRepositoryInformationEvent; import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; +import com.google.devtools.build.lib.vfs.FileStateKey; import com.google.devtools.build.skyframe.CycleInfo; import com.google.devtools.build.skyframe.CyclesReporter; import com.google.devtools.build.skyframe.SkyKey; @@ -84,6 +86,11 @@ public class BzlmodRepoCycleReporter implements CyclesReporter.SingleCycleReport private static final Predicate IS_MODULE_FILE = SkyFunctions.isSkyFunction(SkyFunctions.MODULE_FILE); + private static final Predicate IS_FILE = SkyFunctions.isSkyFunction(FileValue.FILE); + + private static final Predicate IS_FILE_STATE = + SkyFunctions.isSkyFunction(FileStateKey.FILE_STATE); + private static void requestRepoDefinitions( ExtendedEventHandler eventHandler, Iterable repos) { for (SkyKey repo : repos) { @@ -132,12 +139,14 @@ public boolean maybeReportCycle( IS_WORKSPACE_FILE, IS_MODULE_RESOLUTION, IS_DEP_GRAPH, - IS_MODULE_FILE)) + IS_MODULE_FILE, + IS_FILE, + IS_FILE_STATE)) && Iterables.any(cycle, Predicates.or(IS_REPO_RULE, IS_EXTENSION_IMPL))) { StringBuilder cycleMessage = new StringBuilder( - "Circular definition of repositories generated by module extensions and/or .bzl" - + " files:"); + "Circular definition of repositories generated by module extensions or files in" + + " external repositories:"); Iterable repos = Iterables.filter( cycle, @@ -170,6 +179,10 @@ public boolean maybeReportCycle( return "module dependency graph"; } else if (input.argument() instanceof ModuleFileValue.Key) { return "module file of " + input.argument(); + } else if (input instanceof FileValue.Key fileKey) { + return "file " + fileKey.argument(); + } else if (input instanceof FileStateKey fileStateKey) { + return "file state " + fileStateKey.argument(); } else { Preconditions.checkArgument(input.argument() instanceof BzlLoadValue.Key); return ((BzlLoadValue.Key) input.argument()).getLabel().toString(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java index f321be30b265ed..48902fd943ce70 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java @@ -112,8 +112,8 @@ static final class ExternalDirtinessChecker extends SkyValueDirtinessChecker { private final UnionDirtinessChecker checker = createBasicFilesystemDirtinessChecker(); - ExternalDirtinessChecker(ExternalFilesHelper externalFilesHelper, - EnumSet fileTypesToCheck) { + ExternalDirtinessChecker( + ExternalFilesHelper externalFilesHelper, EnumSet fileTypesToCheck) { this.externalFilesHelper = externalFilesHelper; this.fileTypesToCheck = fileTypesToCheck; } @@ -141,7 +141,8 @@ public SkyValueDirtinessChecker.DirtyResult check( @Nullable Version oldMtsv, SyscallCache syscallCache, @Nullable TimestampGranularityMonitor tsgm) { - FileType fileType = externalFilesHelper.getAndNoteFileType((RootedPath) skyKey.argument()); + var rootedPath = (RootedPath) skyKey.argument(); + var fileType = externalFilesHelper.getAndNoteFileType(rootedPath); boolean cacheable = isCacheableType(fileType); SkyValue newValue = checker.createNewValue(skyKey, cacheable ? syscallCache : SyscallCache.NO_CACHE, tsgm); @@ -158,16 +159,13 @@ public SkyValueDirtinessChecker.DirtyResult check( } private static boolean isCacheableType(FileType fileType) { - switch (fileType) { - case INTERNAL: - case EXTERNAL: - case BUNDLED: - return true; - case EXTERNAL_REPO: - case OUTPUT: - return false; - } - throw new AssertionError("Unknown type " + fileType); + return switch (fileType) { + case INTERNAL, EXTERNAL, BUNDLED -> true; + case EXTERNAL_REPO, OUTPUT -> false; + case REPO_CONTENTS_CACHE_DIRS -> + throw new IllegalStateException( + "Repo contents cache dirs are not expected to be checked for dirtiness"); + }; } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java new file mode 100644 index 00000000000000..eea16f29607648 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java @@ -0,0 +1,28 @@ +// Copyright 2026 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.skyframe; + +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.skyframe.SkyValue; +import javax.annotation.Nullable; + +/** + * The value of an environmental variable from an environment (client env, action env or repository + * env). These are invalidated and injected by {@link SequencedSkyframeExecutor}. + * + * @param value the value in the client environment or null if unset in the environment. + */ +@AutoCodec +public record EnvironmentVariableValue(@Nullable String value) implements SkyValue {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java index ca8293f9c6d736..bff32210f4da99 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java @@ -19,20 +19,24 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.BundledFileSystem; import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; -import com.google.devtools.build.lib.rules.repository.RepositoryFunction; -import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; import com.google.devtools.build.lib.util.TestType; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.io.IOException; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; +import javax.annotation.Nullable; /** Common utilities for dealing with paths outside the package roots. */ public class ExternalFilesHelper { @@ -42,6 +46,7 @@ public class ExternalFilesHelper { private final ExternalFileAction externalFileAction; private final BlazeDirectories directories; private final int maxNumExternalFilesToLog; + private final Supplier repoContentsCachePathSupplier; private final AtomicInteger numExternalFilesLogged = new AtomicInteger(0); private static final int MAX_EXTERNAL_FILES_TO_TRACK = 2500; @@ -58,36 +63,56 @@ private ExternalFilesHelper( AtomicReference pkgLocator, ExternalFileAction externalFileAction, BlazeDirectories directories, + Supplier repoContentsCachePathSupplier, int maxNumExternalFilesToLog) { this.pkgLocator = pkgLocator; this.externalFileAction = externalFileAction; this.directories = directories; + this.repoContentsCachePathSupplier = repoContentsCachePathSupplier; this.maxNumExternalFilesToLog = maxNumExternalFilesToLog; } public static ExternalFilesHelper create( AtomicReference pkgLocator, ExternalFileAction externalFileAction, - BlazeDirectories directories) { + BlazeDirectories directories, + Supplier repoContentsCachePathSupplier) { return TestType.isInTest() - ? createForTesting(pkgLocator, externalFileAction, directories) + ? createForTesting( + pkgLocator, externalFileAction, directories, repoContentsCachePathSupplier) : new ExternalFilesHelper( - pkgLocator, externalFileAction, directories, /*maxNumExternalFilesToLog=*/ 100); + pkgLocator, + externalFileAction, + directories, + repoContentsCachePathSupplier, + /* maxNumExternalFilesToLog= */ 100); } public static ExternalFilesHelper createForTesting( AtomicReference pkgLocator, ExternalFileAction externalFileAction, BlazeDirectories directories) { + return createForTesting( + pkgLocator, + externalFileAction, + directories, + /* repoContentsCachePathSupplier= */ () -> null); + } + + public static ExternalFilesHelper createForTesting( + AtomicReference pkgLocator, + ExternalFileAction externalFileAction, + BlazeDirectories directories, + Supplier repoContentsCachePathSupplier) { return new ExternalFilesHelper( pkgLocator, externalFileAction, directories, + repoContentsCachePathSupplier, // These log lines are mostly spam during unit and integration tests. - /*maxNumExternalFilesToLog=*/ 0); + /* maxNumExternalFilesToLog= */ 0); } - /** * The action to take when an external path is encountered. See {@link FileType} for the * definition of "external". @@ -153,14 +178,29 @@ public enum FileType { * RepositoryDirectoryValue is computed. */ EXTERNAL_REPO, + + /** + * The directory containing the repo contents cache entries as well as direct children + * corresponding to individual predeclared input hashes. These directories are created by Bazel + * but may be deleted when users delete the entire repo contents cache. However, they are always + * recreated by Bazel before they are used and/or depended on via Skyframe. They are thus + * immutably present from the perspective of Skyframe and don't require invalidation. + * + *

Note: If these directories ever need to be checked for dirtiness during diffing, they have + * to be made non-cacheable according to {@link + * DirtinessCheckerUtils.ExternalDirtinessChecker#isCacheableType} so that they are not locked + * in as non-existent if they have been removed. This would result in FileValues for files below + * them (the actual repo contents, of type EXTERNAL) being locked in as non-existent too, + * even after a refetch of the repo has added a new cache entry. + */ + REPO_CONTENTS_CACHE_DIRS, } /** - * Thrown by {@link #maybeHandleExternalFile} when an applicable path is processed (see - * {@link ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}. + * Thrown by {@link #maybeHandleExternalFile} when an applicable path is processed (see {@link + * ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}. */ - static class NonexistentImmutableExternalFileException extends Exception { - } + static class NonexistentImmutableExternalFileException extends Exception {} static class ExternalFilesKnowledge { final boolean anyOutputFilesSeen; @@ -199,14 +239,14 @@ void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) { ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() { return new ExternalFilesHelper( - pkgLocator, externalFileAction, directories, maxNumExternalFilesToLog); + pkgLocator, + externalFileAction, + directories, + repoContentsCachePathSupplier, + maxNumExternalFilesToLog); } public FileType getAndNoteFileType(RootedPath rootedPath) { - return getFileTypeAndRepository(rootedPath).getFirst(); - } - - private Pair getFileTypeAndRepository(RootedPath rootedPath) { FileType fileType = detectFileType(rootedPath); if (FileType.EXTERNAL == fileType) { if (nonOutputExternalFilesSeen.size() >= MAX_EXTERNAL_FILES_TO_TRACK) { @@ -221,7 +261,7 @@ private Pair getFileTypeAndRepository(RootedPath roote if (FileType.OUTPUT == fileType) { anyOutputFilesSeen = true; } - return Pair.of(fileType, null); + return fileType; } /** @@ -236,6 +276,12 @@ private FileType detectFileType(RootedPath rootedPath) { if (packageLocator.getPathEntries().contains(rootedPath.getRoot())) { return FileType.INTERNAL; } + var repoContentsCachePath = repoContentsCachePathSupplier.get(); + if (repoContentsCachePath != null + && rootedPath.asPath().startsWith(repoContentsCachePath) + && !rootedPath.asPath().relativeTo(repoContentsCachePath).isMultiSegment()) { + return FileType.REPO_CONTENTS_CACHE_DIRS; + } // The outputBase may be null if we're not actually running a build. Path outputBase = packageLocator.getOutputBase(); if (outputBase == null) { @@ -262,18 +308,17 @@ private FileType detectFileType(RootedPath rootedPath) { @ThreadSafe FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env) throws NonexistentImmutableExternalFileException, IOException, InterruptedException { - Pair pair = getFileTypeAndRepository(rootedPath); - - FileType fileType = Preconditions.checkNotNull(pair.getFirst()); + FileType fileType = Preconditions.checkNotNull(getAndNoteFileType(rootedPath)); switch (fileType) { case BUNDLED: case INTERNAL: + case REPO_CONTENTS_CACHE_DIRS: break; case EXTERNAL: if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) { logger.atInfo().log("Encountered an external path %s", rootedPath); } - // fall through + // fall through case OUTPUT: if (externalFileAction == ExternalFileAction.ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS) { @@ -284,9 +329,50 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment Preconditions.checkState( externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, externalFileAction); - RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env); + addExternalFilesDependencies(rootedPath, env); break; } return fileType; } + + /** + * For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding repo so + * that if the repo is refetched, the {File,DirectoryListing}StateValue's of files underneath will + * be re-evaluated. + * + *

Note that we don't add a dependency on the parent directory at the package root boundary, so + * the only transitive dependencies from files inside the package roots to external files are + * through symlinks. So the upwards transitive closure of external files is small. + */ + private void addExternalFilesDependencies(RootedPath rootedPath, Environment env) + throws InterruptedException { + var repositoryName = getExternalRepoName(rootedPath); + if (repositoryName != null) { + env.getValue(RepositoryDirectoryValue.key(repositoryName)); + } + } + + /** + * For a file of type {@link FileType#EXTERNAL_REPO}, returns the name of the external repository + * it is in or null if the path is not in a valid external repository. + */ + @Nullable + RepositoryName getExternalRepoName(RootedPath rootedPath) { + PathFragment repositoryPath = rootedPath.asPath().relativeTo(getExternalDirectory()); + if (repositoryPath.isEmpty()) { + // We are the top of the repository path (/external), not in an actual external + // repository path. + return null; + } + try { + return RepositoryName.create(repositoryPath.getSegment(0)); + } catch (LabelSyntaxException ignored) { + // The directory of a repository with an invalid name can never exist. + return null; + } + } + + private Path getExternalDirectory() { + return directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION); + } } 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 06e78fc7f8169b..d8a44740b73ae4 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 @@ -1217,7 +1217,7 @@ private CompiledBuildFile compileBuildFile( // read BUILD file Path inputFile = buildFilePath.asPath(); - byte[] buildFileBytes = null; + byte[] buildFileBytes; try { buildFileBytes = buildFileValue.isSpecialFile() diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java new file mode 100644 index 00000000000000..ffddeed475ab4a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java @@ -0,0 +1,112 @@ +// Copyright 2026 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.skyframe; + +import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableSortedMap; +import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +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 com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import javax.annotation.Nullable; + +/** + * Skyframe function that provides the effective value for an environment variable in the context of + * repository rules and module extensions. This checks {@code --repo_env} overrides first, then falls + * back to the client environment. + */ +public final class RepoEnvironmentFunction implements SkyFunction { + @Override + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { + Map repoEnv = PrecomputedValue.REPO_ENV.get(env); + String key = (String) skyKey.argument(); + if (repoEnv.containsKey(key)) { + return new EnvironmentVariableValue(repoEnv.get(key)); + } + // Fall back to the client environment. + ClientEnvironmentValue clientValue = + (ClientEnvironmentValue) env.getValue(ClientEnvironmentFunction.key(key)); + if (clientValue == null) { + return null; + } + return new EnvironmentVariableValue(clientValue.getValue()); + } + + /** Returns the SkyKey to invoke this function for the environment variable {@code variable}. */ + public static Key key(String variable) { + return Key.create(variable); + } + + @VisibleForSerialization + @AutoCodec + static class Key extends AbstractSkyKey { + private static final SkyKeyInterner interner = SkyKey.newInterner(); + + private Key(String arg) { + super(arg); + } + + private static Key create(String arg) { + return interner.intern(new Key(arg)); + } + + @VisibleForSerialization + @AutoCodec.Interner + static Key intern(Key key) { + return interner.intern(key); + } + + @Override + public SkyFunctionName functionName() { + return SkyFunctions.REPOSITORY_ENVIRONMENT_VARIABLE; + } + + @Override + public SkyKeyInterner getSkyKeyInterner() { + return interner; + } + } + + /** + * Returns a map of environment variable key => values, getting them from Skyframe. Returns null + * if and only if some dependencies from Skyframe still need to be resolved. + */ + @Nullable + public static ImmutableSortedMap> getEnvironmentView( + Environment env, Set keys) throws InterruptedException { + var skyKeys = Collections2.transform(keys, RepoEnvironmentFunction::key); + SkyframeLookupResult values = env.getValuesAndExceptions(skyKeys); + if (env.valuesMissing()) { + return null; + } + + var result = ImmutableSortedMap.>naturalOrder(); + for (var key : skyKeys) { + var value = (EnvironmentVariableValue) values.get(key); + if (value == null) { + return null; + } + result.put(key.argument().toString(), Optional.ofNullable(value.value())); + } + return result.buildOrThrow(); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 43820a3c6fc0a5..973f6313718ddb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -79,6 +79,7 @@ import com.google.devtools.build.lib.vfs.FileStateKey; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.ModifiedFileSet; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.DelegatingGraphInconsistencyReceiver; import com.google.devtools.build.skyframe.EmittedEventState; @@ -113,6 +114,7 @@ import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; +import java.util.function.Supplier; import javax.annotation.Nullable; /** @@ -164,6 +166,7 @@ protected SequencedSkyframeExecutor( ImmutableList buildFilesByPriority, ExternalPackageHelper externalPackageHelper, @Nullable SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile, boolean shouldUseRepoDotBazel, SkyKeyStateReceiver skyKeyStateReceiver, @@ -194,6 +197,7 @@ protected SequencedSkyframeExecutor( workspaceInfoFromDiffReceiver, new SequencedRecordingDifferencer(), repositoryHelpersHolder, + repoContentsCachePathSupplier, globUnderSingleDep); } @@ -799,6 +803,7 @@ public static final class Builder { private WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver = (ignored1, ignored2) -> {}; @Nullable private SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder = null; + private Supplier repoContentsCachePathSupplier = () -> null; private Consumer skyframeExecutorConsumerOnInit = skyframeExecutor -> {}; private SkyFunction ignoredPackagePrefixesFunction; private BugReporter bugReporter = BugReporter.defaultInstance(); @@ -837,6 +842,7 @@ public SequencedSkyframeExecutor build() { buildFilesByPriority, externalPackageHelper, repositoryHelpersHolder, + repoContentsCachePathSupplier, actionOnIOExceptionReadingBuildFile, shouldUseRepoDotBazel, skyKeyStateReceiver, @@ -916,6 +922,12 @@ public Builder setRepositoryHelpersHolder( return this; } + @CanIgnoreReturnValue + public Builder setRepoContentsCachePathSupplier(Supplier repoContentsCachePathSupplier) { + this.repoContentsCachePathSupplier = repoContentsCachePathSupplier; + return this; + } + @CanIgnoreReturnValue public Builder setCrossRepositoryLabelViolationStrategy( CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java index 65fda322698437..ac929ffcee429e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java @@ -20,9 +20,11 @@ import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; +import java.util.function.Supplier; import javax.annotation.Nullable; /** A factory of SkyframeExecutors that returns SequencedSkyframeExecutor. */ @@ -39,6 +41,7 @@ public SkyframeExecutor create( ImmutableMap extraSkyFunctions, SyscallCache syscallCache, @Nullable SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver, BugReporter bugReporter) { return BazelSkyframeExecutorConstants.newBazelSkyframeExecutorBuilder() @@ -51,6 +54,7 @@ public SkyframeExecutor create( .setExtraSkyFunctions(extraSkyFunctions) .setSyscallCache(syscallCache) .setRepositoryHelpersHolder(repositoryHelpersHolder) + .setRepoContentsCachePathSupplier(repoContentsCachePathSupplier) .setSkyKeyStateReceiver(skyKeyStateReceiver) .setBugReporter(bugReporter) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 5a06e025aa420c..b7fc0c162a8f9e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -26,6 +26,8 @@ public final class SkyFunctions { static final SkyFunctionName ACTION_SKETCH = SkyFunctionName.createHermetic("ACTION_SKETCH"); public static final SkyFunctionName ACTION_ENVIRONMENT_VARIABLE = SkyFunctionName.createHermetic("ACTION_ENVIRONMENT_VARIABLE"); + public static final SkyFunctionName REPOSITORY_ENVIRONMENT_VARIABLE = + SkyFunctionName.createHermetic("REPOSITORY_ENVIRONMENT_VARIABLE"); public static final SkyFunctionName DIRECTORY_LISTING_STATE = SkyFunctionName.createNonHermetic("DIRECTORY_LISTING_STATE"); public static final SkyFunctionName DIRECTORY_LISTING = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java index 84284a27743514..9c8502a561fd89 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.actions.TestExecException; import com.google.devtools.build.lib.actions.TopLevelOutputException; import com.google.devtools.build.lib.analysis.AnalysisFailureEvent; +import com.google.devtools.build.lib.bazel.bzlmod.ExternalDepsException; import com.google.devtools.build.lib.analysis.ViewCreationFailedException; import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics.TargetCompatibilityCheckException; import com.google.devtools.build.lib.bugreport.BugReport; @@ -540,6 +541,13 @@ && isExecutionCycle(errorInfo.getCycleInfo())) { configurationIdMessage(ctKey.getConfigurationKey()), ((NoSuchThingException) exception).getDetailedExitCode()); analysisRootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, analysisFailedCause); + } else if (exception instanceof ExternalDepsException externalDepsException) { + AnalysisFailedCause analysisFailedCause = + new AnalysisFailedCause( + topLevelLabel, + configurationIdMessage(ctKey.getConfigurationKey()), + externalDepsException.getDetailedExitCode()); + analysisRootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, analysisFailedCause); } else if (exception instanceof TargetCompatibilityCheckException) { analysisRootCauses = NestedSetBuilder.emptySet(Order.STABLE_ORDER); } else if (isExecutionException(exception)) { @@ -779,7 +787,8 @@ private static DetailedException convertToAnalysisException(Throwable cause) { // analyze with --nokeep_going. if (cause instanceof SaneAnalysisException || cause instanceof NoSuchTargetException - || cause instanceof NoSuchPackageException) { + || cause instanceof NoSuchPackageException + || cause instanceof ExternalDepsException) { return (DetailedException) cause; } return null; 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 1d21fe069c48fe..79c92ea8506b92 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 @@ -566,6 +566,7 @@ protected SkyframeExecutor( @Nullable WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver, @Nullable RecordingDifferencer recordingDiffer, @Nullable SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, boolean globUnderSingleDep) { // Strictly speaking, these arguments are not required for initialization, but all current // callsites have them at hand, so we might as well set them during construction. @@ -609,7 +610,8 @@ protected SkyframeExecutor( this.skyframeBuildView = new SkyframeBuildView(artifactFactory, this, ruleClassProvider, actionKeyContext); this.externalFilesHelper = - ExternalFilesHelper.create(pkgLocator, externalFileAction, directories); + ExternalFilesHelper.create( + pkgLocator, externalFileAction, directories, repoContentsCachePathSupplier); this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy; this.buildFilesByPriority = buildFilesByPriority; this.externalPackageHelper = externalPackageHelper; @@ -638,6 +640,7 @@ private ImmutableMap skyFunctions() { map.put(SkyFunctions.PRECOMPUTED, new PrecomputedFunction()); map.put(SkyFunctions.CLIENT_ENVIRONMENT_VARIABLE, new ClientEnvironmentFunction(clientEnv)); map.put(SkyFunctions.ACTION_ENVIRONMENT_VARIABLE, new ActionEnvironmentFunction()); + map.put(SkyFunctions.REPOSITORY_ENVIRONMENT_VARIABLE, new RepoEnvironmentFunction()); map.put(FileStateKey.FILE_STATE, newFileStateFunction()); map.put(SkyFunctions.DIRECTORY_LISTING_STATE, newDirectoryListingStateFunction()); map.put(FileSymlinkCycleUniquenessFunction.NAME, new FileSymlinkCycleUniquenessFunction()); @@ -1920,19 +1923,17 @@ public BuildConfigurationValue getConfiguration( Map.Entry firstError = Iterables.get(evalResult.errorMap().entrySet(), 0); ErrorInfo error = firstError.getValue(); Throwable e = error.getException(); - // Wrap loading failed exceptions - if (e != null && e instanceof NoSuchThingException noSuchThingException) { - e = new InvalidConfigurationException(noSuchThingException.getDetailedExitCode(), e); + if (e instanceof InvalidConfigurationException invalidConfigurationException) { + throw invalidConfigurationException; + } else if (e instanceof DetailedException detailedException) { + throw new InvalidConfigurationException(detailedException.getDetailedExitCode(), e); } else if (e == null && !error.getCycleInfo().isEmpty()) { cyclesReporter.reportCycles(error.getCycleInfo(), firstError.getKey(), eventHandler); - e = - new InvalidConfigurationException( + throw new InvalidConfigurationException( "cannot load build configuration because of this cycle", Code.CYCLE); } - if (e != null) { - Throwables.throwIfInstanceOf(e, InvalidConfigurationException.class); - } - throw new IllegalStateException("Unknown error during configuration creation evaluation", e); + throw new IllegalStateException( + "Unknown error during configuration creation evaluation", e); } // Prepare and return the results. @@ -3469,6 +3470,8 @@ protected void handleDiffsWithMissingDiffInformation( fileTypesToCheck.add(FileType.OUTPUT); } if (!fileTypesToCheck.isEmpty()) { + // FileType.REPO_CONTENTS_CACHE_DIRS is intentionally never checked here. See the comment on + // that enum constant for details. dirtinessCheckers = Iterables.concat( dirtinessCheckers, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java index ce922b4676b56f..aee3b3fa16ad9c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java @@ -21,9 +21,11 @@ import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; +import java.util.function.Supplier; /** * A factory that creates instances of SkyframeExecutor. @@ -52,6 +54,7 @@ SkyframeExecutor create( ImmutableMap extraSkyFunctions, SyscallCache syscallCache, SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver, BugReporter bugReporter) throws AbruptExitException; 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 383eaabb377801..c3f4a3d80a179e 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 @@ -283,7 +283,11 @@ protected void validate() { public final PackageLoader build() { validate(); externalFilesHelper = - ExternalFilesHelper.create(pkgLocatorRef, externalFileAction, directories); + ExternalFilesHelper.create( + pkgLocatorRef, + externalFileAction, + directories, + /* repoContentsCachePathSupplier= */ () -> null); return buildImpl(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD index a86993c2995fba..5bb1b2b50084a8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD @@ -102,6 +102,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java index 4bf99d967d5a91..da5bd0113f6daa 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java @@ -48,6 +48,7 @@ import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.PrecomputedValue; +import com.google.devtools.build.lib.skyframe.RepoEnvironmentFunction; import com.google.devtools.build.lib.skyframe.RepositoryMappingFunction; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.vfs.Path; @@ -198,6 +199,7 @@ public void handle(Event event) {} SkyFunctions.DIRECTORY_LISTING_STATE, new DirectoryListingStateFunction(externalFilesHelper, SyscallCache.NO_CACHE)) .put(SkyFunctions.ACTION_ENVIRONMENT_VARIABLE, new ActionEnvironmentFunction()) + .put(SkyFunctions.REPOSITORY_ENVIRONMENT_VARIABLE, new RepoEnvironmentFunction()) .put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()) .put( SkyFunctions.LOCAL_REPOSITORY_LOOKUP, diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java index c5c93fe88afe00..160fa4e1e3ce30 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java @@ -929,7 +929,8 @@ public void prefetchPackageAsync(int maxDirs) { private void checkSameFileSystem(Path that) { if (this.fileSystem != that.fileSystem) { throw new IllegalArgumentException( - "Files are on different filesystems: " + this + ", " + that); + "Files are on different filesystems: %s (on %s), %s (on %s)" + .formatted(this, this.fileSystem, that, that.fileSystem)); } } } diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 5691ff1dbd541c..f1001b715a9f18 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -544,6 +544,7 @@ message Filesystem { [(metadata) = { exit_code: 2 }]; FILESYSTEM_JNI_NOT_AVAILABLE = 8 [(metadata) = { exit_code: 36 }]; FAILED_TO_LOCK_INSTALL_BASE = 12 [(metadata) = { exit_code: 36 }]; + REMOTE_FILE_EVICTED = 13 [(metadata) = { exit_code: 39 }]; reserved 7, 9, 10; // For internal use } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java index a3a5fcb196305e..dacd20c70bf02a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java @@ -36,7 +36,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.BazelCompatibilityMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction; import com.google.devtools.build.lib.packages.util.LoadingMock; import com.google.devtools.build.lib.packages.util.MockCcSupport; @@ -221,7 +221,7 @@ public ImmutableMap getSkyFunctions(BlazeDirectori ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())) + new LocalRepoContentsCache())) .put( SkyFunctions.MODULE_FILE, new ModuleFileFunction( diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java index 2f409e94e40538..1b3de52f088aeb 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; import java.util.Optional; @@ -45,18 +46,14 @@ public void setUp() throws Exception { LockFileModuleExtension.builder() .setBzlTransitiveDigest(new byte[] {1, 2, 3}) .setUsagesDigest(new byte[] {4, 5, 6}) - .setRecordedFileInputs(ImmutableMap.of()) - .setRecordedDirentsInputs(ImmutableMap.of()) - .setEnvVariables(ImmutableMap.of()) + .setRecordedInputs(ImmutableList.of()) .setGeneratedRepoSpecs(ImmutableMap.of()) .build(); reproducibleResult = LockFileModuleExtension.builder() .setBzlTransitiveDigest(new byte[] {1, 2, 3}) .setUsagesDigest(new byte[] {4, 5, 6}) - .setRecordedFileInputs(ImmutableMap.of()) - .setRecordedDirentsInputs(ImmutableMap.of()) - .setEnvVariables(ImmutableMap.of()) + .setRecordedInputs(ImmutableList.of()) .setGeneratedRepoSpecs(ImmutableMap.of()) .setModuleExtensionMetadata( LockfileModuleExtensionMetadata.of( diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java index ae5e4fe1a9c6fd..2cc4c5f510a266 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java @@ -32,7 +32,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.BazelCompatibilityMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; @@ -168,7 +168,7 @@ public void setup() throws Exception { ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())) + new LocalRepoContentsCache())) .put( BzlmodRepoRuleValue.BZLMOD_REPO_RULE, new BzlmodRepoRuleFunction(ruleClassProvider, directories)) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java index 6b5974f6b0e0f6..8696c62d8e03e7 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java @@ -32,7 +32,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.InterimModuleBuilder; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; @@ -200,7 +200,7 @@ private void setUpWithBuiltinModules(ImmutableMap b ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())) + new LocalRepoContentsCache())) .put( BzlmodRepoRuleValue.BZLMOD_REPO_RULE, new BzlmodRepoRuleFunction(ruleClassProvider, directories)) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 36d2c3440a2933..7abe7a19a478ad 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -34,7 +34,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.BazelCompatibilityMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; @@ -245,7 +245,7 @@ public void setup() throws Exception { ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())) + new LocalRepoContentsCache())) .put( BzlmodRepoRuleValue.BZLMOD_REPO_RULE, new BzlmodRepoRuleFunction(ruleClassProvider, directories)) @@ -1804,7 +1804,7 @@ public void testReportRepoAndBzlCycles_circularExtReposCtxRead() throws Exceptio assertContainsEvent( """ ERROR : Circular definition of repositories generated by module extensions\ - and/or .bzl files: + or files in external repositories: .-> @@+my_ext+candy1 | module extension @@//:defs.bzl%my_ext | @@+my_ext2+candy2 @@ -1851,7 +1851,7 @@ public void testReportRepoAndBzlCycles_circularExtReposLoadInDefFile() throws Ex assertContainsEvent( """ ERROR : Circular definition of repositories generated by module extensions\ - and/or .bzl files: + or files in external repositories: .-> @@+my_ext+candy1 | module extension @@//:defs.bzl%my_ext | @@+my_ext2+candy2 @@ -1891,7 +1891,7 @@ public void testReportRepoAndBzlCycles_extRepoLoadSelfCycle() throws Exception { assertContainsEvent( """ ERROR : Circular definition of repositories generated by module extensions\ - and/or .bzl files: + or files in external repositories: .-> @@+my_ext+candy1 | module extension @@//:defs.bzl%my_ext | //:defs.bzl diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index dcc24ba0bda993..2f652784c6e074 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -34,7 +34,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.InterimModuleBuilder; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.LabelConstants; @@ -181,7 +181,7 @@ private void setUpWithBuiltinModules(ImmutableMap b ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())) + new LocalRepoContentsCache())) .put( BzlmodRepoRuleValue.BZLMOD_REPO_RULE, new BzlmodRepoRuleFunction(ruleClassProvider, directories)) diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 074eaa343fc098..8bc52454b3fcea 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -81,6 +81,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/actions:runfiles_metadata", "//src/main/java/com/google/devtools/build/lib/actions:runfiles_tree", + "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", @@ -145,6 +146,7 @@ java_library( "//src/test/java/com/google/devtools/build/lib/exec/util", "//src/test/java/com/google/devtools/build/lib/remote/util", "//src/test/java/com/google/devtools/build/lib/testutil", + "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:auth", "//third_party:caffeine", diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index dba0c00671e76b..49cd10aec1e20c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -701,7 +701,8 @@ private ActionResult upload( outErr, /* exitCode= */ 0, /* startTime= */ null, - /* wallTimeInMs= */ 0); + /* wallTimeInMs= */ 0, + /* preserveExecutableBit= */ false); return uploadManifest.upload(context, combinedCache, NullEventHandler.INSTANCE); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java index e3ae3b4e28868a..440dc088264be8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java @@ -34,6 +34,7 @@ import com.google.common.eventbus.EventBus; import com.google.common.truth.extensions.proto.ProtoTruth; import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; @@ -49,6 +50,7 @@ import com.google.devtools.build.lib.remote.disk.DiskCacheGarbageCollectorIdleTask; import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader; import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions; import com.google.devtools.build.lib.runtime.BlazeWorkspace; @@ -61,6 +63,7 @@ import com.google.devtools.build.lib.runtime.commands.BuildCommand; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -167,6 +170,13 @@ private static CommandEnvironment createTestCommandEnvironment( .addBlazeModule(new CredentialModule()) .addBlazeModule(remoteModule) .addBlazeModule(new BlockWaitingModule()) + .addBlazeModule( + new BlazeModule() { + @Override + public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) { + builder.setRunfilesPrefix(TestConstants.WORKSPACE_NAME); + } + }) .build(); BlazeDirectories directories = diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java index 0d03dbe9dc405e..62afde6b98a8d0 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java @@ -760,4 +760,102 @@ private Path createDirectoryWithSymlinkToSpecialFile( return dir; } + + @Test + public void actionResult_preserveExecutableBit_executableFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path file = execRoot.getRelative("file"); + FileSystemUtils.writeContent(file, new byte[] {1, 2, 3, 4, 5}); + file.setExecutable(true); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /* allowAbsoluteSymlinks= */ false, + /* preserveExecutableBit= */ true); + um.addFiles(ImmutableList.of(file)); + Digest digest = digestUtil.compute(file); + assertThat(um.getDigestToFile()).containsExactly(digest, file); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("file").setDigest(digest).setIsExecutable(true); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_preserveExecutableBit_nonExecutableFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path file = execRoot.getRelative("file"); + FileSystemUtils.writeContent(file, new byte[] {1, 2, 3, 4, 5}); + file.setExecutable(false); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /* allowAbsoluteSymlinks= */ false, + /* preserveExecutableBit= */ true); + um.addFiles(ImmutableList.of(file)); + Digest digest = digestUtil.compute(file); + assertThat(um.getDigestToFile()).containsExactly(digest, file); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("file").setDigest(digest).setIsExecutable(false); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_preserveExecutableBit_mixedFilesInDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path executableFile = execRoot.getRelative("dir/executable"); + FileSystemUtils.writeContent(executableFile, new byte[] {1, 2, 3}); + executableFile.setExecutable(true); + Path nonExecutableFile = execRoot.getRelative("dir/nonexecutable"); + FileSystemUtils.writeContent(nonExecutableFile, new byte[] {4, 5, 6}); + nonExecutableFile.setExecutable(false); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /* allowAbsoluteSymlinks= */ false, + /* preserveExecutableBit= */ true); + um.addFiles(ImmutableList.of(dir)); + + Digest executableDigest = digestUtil.compute(executableFile); + Digest nonExecutableDigest = digestUtil.compute(nonExecutableFile); + assertThat(um.getDigestToFile()) + .containsExactly(executableDigest, executableFile, nonExecutableDigest, nonExecutableFile); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setName("executable") + .setDigest(executableDigest) + .setIsExecutable(true)) + .addFiles( + FileNode.newBuilder() + .setName("nonexecutable") + .setDigest(nonExecutableDigest) + .setIsExecutable(false))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } } 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 5809cdde7ae453..62a84b7397d807 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 @@ -46,7 +46,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.BazelCompatibilityMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; @@ -146,7 +146,7 @@ public void setupDelegator() throws Exception { /* clientEnvironmentSupplier= */ ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache()); + new LocalRepoContentsCache()); AtomicReference pkgLocator = new AtomicReference<>( new PathPackageLocator( diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java index 0318317ae4e62a..6dd973b6fc93a6 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java @@ -15,8 +15,11 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.rules.repository.RepoRecordedInput.WithValue.parse; +import static com.google.devtools.build.lib.rules.repository.RepoRecordedInput.WithValue.splitIntoBatches; import static org.mockito.Mockito.when; +import com.google.common.collect.ImmutableList; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileContentsProxy; import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithContentsProxy; @@ -71,4 +74,22 @@ public void testFileValueToMarkerValue() throws Exception { String expectedDigest = BaseEncoding.base16().lowerCase().encode(path.asPath().getDigest()); assertThat(RepoRecordedInput.File.fileValueToMarkerValue(path, fv)).isEqualTo(expectedDigest); } + + @Test + public void testSplitIntoBatches() { + assertThat(splitIntoBatches(ImmutableList.of())).isEmpty(); + assertThat( + splitIntoBatches( + ImmutableList.of( + parse("FILE:@@//foo:bar abc").orElseThrow(), + parse("FILE:@@//:baz cba").orElseThrow(), + parse("FILE:@@foo//:baz bac").orElseThrow(), + parse("ENV:KEY value").orElseThrow()))) + .containsExactly( + ImmutableList.of( + parse("FILE:@@//foo:bar abc").orElseThrow(), + parse("FILE:@@//:baz cba").orElseThrow()), + ImmutableList.of( + parse("FILE:@@foo//:baz bac").orElseThrow(), parse("ENV:KEY value").orElseThrow())); + } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java index e570d188df408f..8e40ed66dd66b8 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java @@ -313,6 +313,7 @@ private void initEvaluator() getExtraSkyFunctions(), SyscallCache.NO_CACHE, /* repositoryHelpersHolder= */ null, + /* repoContentsCachePathSupplier= */ () -> null, SkyframeExecutor.SkyKeyStateReceiver.NULL_INSTANCE, BugReporter.defaultInstance()); skyframeExecutor.injectExtraPrecomputedValues( 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 5f3109c0b8ebcb..d446803ebbeb41 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 @@ -26,7 +26,7 @@ import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -154,7 +154,7 @@ public final void setUp() throws Exception { ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())); + new LocalRepoContentsCache())); skyFunctions.put( SkyFunctions.REPOSITORY_MAPPING, new SkyFunction() { 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 3c3cee8dc7bf80..7efa8b5ebbe43f 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 @@ -41,7 +41,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.events.NullEventHandler; @@ -219,7 +219,7 @@ private MemoizingEvaluator makeEvaluator(ExternalFileAction externalFileAction) ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())) + new LocalRepoContentsCache())) .put( SkyFunctions.REPOSITORY_MAPPING, new SkyFunction() { 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 2c9f7067188be7..8c6d588f494767 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 @@ -29,7 +29,7 @@ import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; -import com.google.devtools.build.lib.bazel.repository.cache.RepoContentsCache; +import com.google.devtools.build.lib.bazel.repository.cache.LocalRepoContentsCache; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -169,7 +169,7 @@ public final void setUp() throws Exception { ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, - new RepoContentsCache())); + new LocalRepoContentsCache())); skyFunctions.put( SkyFunctions.REPOSITORY_MAPPING, new SkyFunction() { diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD index 4d7e56996f5c9b..97290e73a9e179 100644 --- a/src/test/py/bazel/BUILD +++ b/src/test/py/bazel/BUILD @@ -415,6 +415,19 @@ py_test( name = "repo_contents_cache_test", size = "large", srcs = ["bzlmod/repo_contents_cache_test.py"], + shard_count = 4, + tags = ["requires-network"], + deps = [ + ":bzlmod_test_utils", + ":test_base", + ], +) + +py_test( + name = "remote_repo_contents_cache_test", + size = "large", + srcs = ["bzlmod/remote_repo_contents_cache_test.py"], + shard_count = 2, tags = ["requires-network"], deps = [ ":bzlmod_test_utils", diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index ed092907c07e8a..9aef1ec859e7c6 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/src/test/py/bazel/bzlmod/bazel_module_test.py @@ -1540,6 +1540,96 @@ def testUnknownRegistryInModuleMirrors(self): ) self.assertIn('https://unknown-registry.example.com', stderr) + def testInvalidRepoRuleReferencedByTargetDoesNotCrash(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile( + 'BUILD', + [ + 'genrule(', + ' name = "gen",', + ' srcs = ["@my_repo//:a.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $(SRCS) > $(OUTS)",', + ')', + ], + ) + self.ScratchFile('repo.bzl', ['nonsense']) + + exit_code, _, stderr = self.RunBazel( + ['build', '//:gen'], + allow_failure=True, + ) + self.AssertNotExitCode(exit_code, 0, stderr) + stderr = '\n'.join(stderr) + self.assertNotIn('FATAL', stderr) + self.assertIn("compilation of module 'repo.bzl' failed", stderr) + + def testReverseDependencyDirection(self): + # Set up two module extensions that retain their predeclared input hashes + # across two builds but still reverse their dependency direction. Depending + # on how repo cache candidates are checked, this could lead to a Skyframe + # cycle. + self.ScratchFile( + 'MODULE.bazel', + [ + 'foo_ext = use_extension("//:repo.bzl", "foo_ext")', + 'use_repo(foo_ext, "foo")', + 'bar_ext = use_extension("//:repo.bzl", "bar_ext")', + 'use_repo(bar_ext, "bar")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("output.txt", rctx.attr.content)', + ' rctx.file("BUILD", "exports_files([\'output.txt\'])")', + ' print("JUST FETCHED: %s" % rctx.original_name)', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(', + ' implementation = _repo_impl,', + ' attrs = {"content": attr.string()},', + ')', + 'def _foo_ext_impl(mctx):', + ' deps = mctx.read(Label("@//:foo_deps.txt")).splitlines()', + ' content = "foo"', + ' for dep in deps:', + ' if dep:', + ' content += " + " + mctx.read(Label(dep))', + ' repo(name = "foo", content = content)', + 'foo_ext = module_extension(implementation = _foo_ext_impl)', + 'def _bar_ext_impl(mctx):', + ' deps = mctx.read(Label("@//:bar_deps.txt")).splitlines()', + ' content = "bar"', + ' for dep in deps:', + ' if dep:', + ' content += " + " + mctx.read(Label(dep))', + ' repo(name = "bar", content = content)', + 'bar_ext = module_extension(implementation = _bar_ext_impl)', + ], + ) + + self.ScratchFile('foo_deps.txt', ['@bar//:output.txt']) + self.ScratchFile('bar_deps.txt') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@foo//:output.txt']) + self.assertIn('JUST FETCHED: bar', '\n'.join(stderr)) + self.assertIn('JUST FETCHED: foo', '\n'.join(stderr)) + + # After expunging and reversing: cached + self.RunBazel(['clean', '--expunge']) + self.ScratchFile('foo_deps.txt') + self.ScratchFile('bar_deps.txt', ['@foo//:output.txt']) + self.RunBazel(['build', '@foo//:output.txt']) + if __name__ == '__main__': absltest.main() diff --git a/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py new file mode 100644 index 00000000000000..a223df321a6afe --- /dev/null +++ b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py @@ -0,0 +1,803 @@ +# pylint: disable=g-backslash-continuation +# Copyright 2025 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# pylint: disable=g-long-ternary +# pylint: disable=g-bad-todo + +import json +import os +import re +import tempfile +from absl.testing import absltest +from src.test.py.bazel import test_base + + +class RemoteRepoContentsCacheTest(test_base.TestBase): + + def setUp(self): + test_base.TestBase.setUp(self) + worker_port = self.StartRemoteWorker() + self.ScratchFile( + '.bazelrc', + [ + 'startup --experimental_remote_repo_contents_cache', + # Only use the remote repo contents cache. + 'common --repo_contents_cache=', + 'common --remote_cache=grpc://localhost:' + str(worker_port), + 'common --auth_enabled=false', + 'common --remote_timeout=3600s', + 'common --verbose_failures', + ], + ) + + def tearDown(self): + test_base.TestBase.tearDown(self) + self.StopRemoteWorker() + + def RepoDir(self, repo_name, cwd=None): + _, stdout, _ = self.RunBazel(['info', 'output_base'], cwd=cwd) + self.assertLen(stdout, 1) + output_base = stdout[0].strip() + + _, stdout, _ = self.RunBazel(['mod', 'dump_repo_mapping', ''], cwd=cwd) + self.assertLen(stdout, 1) + mapping = json.loads(stdout[0]) + canonical_repo_name = mapping[repo_name] + + return output_base + '/external/' + canonical_repo_name + + def testCachedAfterCleanExpunge(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging, without using repo contents cache: not cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel([ + '--noexperimental_remote_repo_contents_cache', + 'build', + '@my_repo//:haha', + ]) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testLocalRepoContentsCacheInteraction(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + repo_contents_cache = tempfile.mkdtemp(dir=os.environ['TEST_TMPDIR']) + _, _, stderr = self.RunBazel([ + 'build', + '@my_repo//:haha', + '--repo_contents_cache=' + repo_contents_cache, + ]) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: cached, hits the local repo contents cache + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel([ + 'build', + '@my_repo//:haha', + '--repo_contents_cache=' + repo_contents_cache, + ]) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After cleaning out local repo contents cache: cached, hits the remote + # cache + self.RunBazel(['clean', '--expunge']) + # Deleting the cache fails on Windows, so we just use a different directory. + _, _, stderr = self.RunBazel([ + 'build', + '@my_repo//:haha', + '--repo_contents_cache=' + repo_contents_cache + '2', + ]) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging, without using any repo contents cache: not cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel([ + '--noexperimental_remote_repo_contents_cache', + 'build', + '@my_repo//:haha', + ]) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNotCachedWhenPredeclaredInputsChange(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo", data = 1)', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl, attrs={"data":attr.int()})', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change predeclared inputs: not cached + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo", data = 2)', + ], + ) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change back to previous predeclared inputs: cached (even after expunging) + self.RunBazel(['clean', '--expunge']) + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo", data = 1)', + ], + ) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNotCachedWhenRecordedInputsChange_dynamicDep(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' rctx.watch(Label("@//:data.txt"))', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + self.ScratchFile('data.txt', ['one']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change recorded inputs: not cached + self.ScratchFile('data.txt', ['two']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change back to previous recorded inputs: not cached + # TODO: This is the current behavior, but it's not desired. Support for + # caching repos with dynamic deps should be added. + self.RunBazel(['clean', '--expunge']) + self.ScratchFile('data.txt', ['one']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNotCachedWhenRecordedInputsChange_staticDep(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' rctx.os.environ.get("LOLOL")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl, environ = ["LOLOL"])', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], env_add={'LOLOL': 'lol'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change recorded inputs: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], env_add={'LOLOL': 'kek'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change back to previous recorded inputs: cached (even after expunging) + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], env_add={'LOLOL': 'lol'} + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNoThrashingBetweenWorkspaces(self): + module_bazel_lines = [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ] + repo_bzl_lines = [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' rctx.os.environ.get("LOLOL")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl, environ = ["LOLOL"])', + ] + # Set up two workspaces with exactly the same repo definition (same + # predeclared inputs) + dir_a = self.ScratchDir('a') + dir_b = self.ScratchDir('b') + self.ScratchFile('a/MODULE.bazel', module_bazel_lines) + self.ScratchFile('b/MODULE.bazel', module_bazel_lines) + self.ScratchFile('a/BUILD.bazel') + self.ScratchFile('b/BUILD.bazel') + self.ScratchFile('a/repo.bzl', repo_bzl_lines) + self.ScratchFile('b/repo.bzl', repo_bzl_lines) + self.CopyFile(self.Path('.bazelrc'), 'a/.bazelrc') + self.CopyFile(self.Path('.bazelrc'), 'b/.bazelrc') + + repo_dir_a = self.RepoDir('my_repo', cwd=dir_a) + repo_dir_b = self.RepoDir('my_repo', cwd=dir_b) + + # First fetch in A: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_a, env_add={'LOLOL': 'lol'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir_a, 'BUILD'))) + + # Fetch in B (with same env variable): cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_b, env_add={'LOLOL': 'lol'} + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir_b, 'BUILD'))) + + # Change env variable: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_b, env_add={'LOLOL': 'rofl'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir_b, 'BUILD'))) + + # Building A again even after expunging: cached + self.RunBazel(['clean', '--expunge'], cwd=dir_a) + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_a, env_add={'LOLOL': 'rofl'} + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir_a, 'BUILD'))) + + def testAccessFromOtherRepo_read(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + 'other_repo = use_repo_rule("//:other_repo.bzl", "other_repo")', + 'other_repo(name = "other", build_file = "@my_repo//:BUILD")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + # Verify that directories are materialized correctly. + ' rctx.file("subdir/file.txt", "hello")', + ' rctx.file("subdir/empty_dir/.keep")', + ' rctx.delete("subdir/empty_dir/.keep")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'other_repo.bzl', + [ + 'def _other_repo_impl(rctx):', + ' rctx.file("BUILD", rctx.read(rctx.path(rctx.attr.build_file)))', + ' return rctx.repo_metadata()', + ( + 'other_repo = repository_rule(_other_repo_impl,' + ' attrs={"build_file": attr.label()})' + ), + ], + ) + + repo_dir = self.RepoDir('my_repo') + other_repo_dir = self.RepoDir('other') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: fetch my_repo only, not materialized + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'subdir'))) + + # Fetch other: my_repo materialized + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/file.txt'))) + with open(os.path.join(repo_dir, 'subdir/file.txt')) as f: + self.assertEqual(f.read(), 'hello') + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/empty_dir'))) + + # Materialized repo is not refetched after a shutdown + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + def testAccessFromOtherRepo_symlink(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + 'other_repo = use_repo_rule("//:other_repo.bzl", "other_repo")', + 'other_repo(name = "other", build_file = "@my_repo//:BUILD")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'other_repo.bzl', + [ + 'def _other_repo_impl(rctx):', + ' rctx.symlink(rctx.path(rctx.attr.build_file), "BUILD")', + ' return rctx.repo_metadata()', + ( + 'other_repo = repository_rule(_other_repo_impl,' + ' attrs={"build_file": attr.label()})' + ), + ], + ) + + repo_dir = self.RepoDir('my_repo') + other_repo_dir = self.RepoDir('other') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: fetch my_repo only, not materialized + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Fetch other: my_repo materialized + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + # Materialized repo is not refetched after a shutdown + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + def testUseRepoFileInBuildRule_actionUsesCache(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "exports_files([\'data.txt\'])")', + ' rctx.file("data.txt", "hello")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'main/BUILD.bazel', + [ + 'genrule(', + ' name = "use_data",', + ' srcs = ["@my_repo//:data.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $< > $@",', + ')', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '//main:use_data']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + # After expunging: repo and build action cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '//main:use_data']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + def do_testUseRepoFileInBuildRule_actionDoesNotUseCache( + self, extra_flags=None + ): + if extra_flags is None: + extra_flags = [] + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "exports_files([\'data.txt\'])")', + ' rctx.file("data.txt", "hello")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'main/BUILD.bazel', + [ + 'genrule(', + ' name = "use_data",', + ' srcs = ["@my_repo//:data.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $< > $@",', + ' tags = ["no-cache"],', + ')', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + # After expunging: repo and build action cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): + self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache() + + def testUseRepoFileInBuildRule_actionDoesNotUseCache_withExplicitSandboxBase( + self, + ): + tmpdir = self.ScratchDir('sandbox_base') + self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache( + extra_flags=['--sandbox_base=' + tmpdir] + ) + + def testLostRemoteFile_build(self): + # Create a repo with two BUILD files (one in a subpackage), build a target + # from one to cause it to be cached, then build that target again after + # expunging to verify it is cached. + # Then, restart the worker and build a target in the other build file. + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ( + ' rctx.file("BUILD", "filegroup(name=\'root\',' + " srcs=['root.txt'])\")" + ), + ' rctx.file("root.txt", "root")', + ( + ' rctx.file("sub/BUILD", "filegroup(name=\'sub\',' + " srcs=['sub.txt'])\")" + ), + ' rctx.file("sub/sub.txt", "sub")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:root']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + # After expunging: cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:root']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + # Lose all remote files. + self.ClearRemoteCache() + + # Build the other target: fails due to the lost input + _, _, stderr = self.RunBazel(['build', '@my_repo//sub:sub']) + # First restart recovers @my_repo, the next one recovers @platforms. + self.assertEqual( + 2, + stderr.count( + 'Found transient remote cache error, retrying the build...' + ), + ) + canonical_repo_name = repo_dir[repo_dir.rfind('/') + 1 :] + stderr = '\n'.join(stderr) + self.assertRegex( + stderr, + 'external/%s/sub/BUILD with digest .*/.* no longer available in the' + ' remote cache' + % re.escape(canonical_repo_name), + ) + self.assertIn('JUST FETCHED', stderr) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + # After expunging again: cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//sub:sub']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + def testBzlFilePrefetching(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", """', + 'load(":nested.bzl", "nested_fg")', + 'nested_fg(name = "haha")', + '""")', + ' rctx.file("nested.bzl", """', + 'load("//subdir:more_nested.bzl", "more_nested_fg")', + 'def nested_fg(name):', + ' more_nested_fg(name = name)', + '""")', + ' rctx.file("subdir/BUILD")', + ' rctx.file("subdir/more_nested.bzl", """', + 'def more_nested_fg(name):', + ' native.filegroup(name = name)', + '""")', + ' rctx.file("file.txt", "hello")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'file.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'nested.bzl'))) + self.assertTrue( + os.path.exists(os.path.join(repo_dir, 'subdir/more_nested.bzl')) + ) + + # After expunging: cached, .bzl files materialized + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'file.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'subdir/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'nested.bzl'))) + self.assertTrue( + os.path.exists(os.path.join(repo_dir, 'subdir/more_nested.bzl')) + ) + + # After expunging, without using repo contents cache: not cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel([ + '--noexperimental_remote_repo_contents_cache', + 'build', + '@my_repo//:haha', + ]) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'file.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'nested.bzl'))) + self.assertTrue( + os.path.exists(os.path.join(repo_dir, 'subdir/more_nested.bzl')) + ) + + def testRun(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "buildozer", version = "8.5.1")', + ], + ) + + # First fetch: not cached + _, stdout, _ = self.RunBazel(['run', '@buildozer', '--', '--version']) + self.assertIn('buildozer version: 8.5.1', '\n'.join(stdout)) + + # After expunging: cached + self.RunBazel(['clean', '--expunge']) + _, stdout, _ = self.RunBazel(['run', '@buildozer', '--', '--version']) + self.assertIn('buildozer version: 8.5.1', '\n'.join(stdout)) + repo_dir = self.RepoDir('buildozer') + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'MODULE.bazel'))) + + +if __name__ == '__main__': + absltest.main() diff --git a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py index ff121051e880c3..3d4791ab1953ed 100644 --- a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py @@ -14,9 +14,12 @@ # limitations under the License. # pylint: disable=g-long-ternary +import json import os +import pathlib import tempfile import time +import unittest from absl.testing import absltest from src.test.py.bazel import test_base @@ -54,6 +57,18 @@ def sleepUntilCacheEmpty(self): time.sleep(0.5) self.fail('repo contents cache still not empty after 5 seconds') + def repoDir(self, repo_name, cwd=None): + _, stdout, _ = self.RunBazel(['info', 'output_base'], cwd=cwd) + self.assertLen(stdout, 1) + output_base = stdout[0].strip() + + _, stdout, _ = self.RunBazel(['mod', 'dump_repo_mapping', ''], cwd=cwd) + self.assertLen(stdout, 1) + mapping = json.loads(stdout[0]) + canonical_repo_name = mapping[repo_name] + + return output_base + '/external/' + canonical_repo_name + def testCachedAfterCleanExpunge(self): self.ScratchFile( 'MODULE.bazel', @@ -76,6 +91,21 @@ def testCachedAfterCleanExpunge(self): # First fetch: not cached _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) self.assertIn('JUST FETCHED', '\n'.join(stderr)) + # Verify that the repo directory under the output base is a symlink or + # junction into the repo contents cache. + repo_dir = self.repoDir('my_repo') + self.assertTrue(os.path.islink(repo_dir) or os.path.isjunction(repo_dir)) + target_path = os.readlink(repo_dir) + real_target_path = os.path.realpath(target_path) + real_repo_contents_cache = os.path.realpath(self.repo_contents_cache) + for parent in pathlib.Path(real_target_path).parents: + if parent.samefile(real_repo_contents_cache): + break + else: + self.fail( + 'repo target dir %s is not in the repo contents cache %s' + % (real_target_path, real_repo_contents_cache) + ) # After expunging: cached self.RunBazel(['clean', '--expunge']) @@ -295,7 +325,9 @@ def testGc_singleServer_gcAfterCacheHit(self): # GC'd while server is alive: not cached, but also no crash self.sleepUntilCacheEmpty() _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) def testGc_singleServer_gcAfterCacheMiss(self): self.ScratchFile( @@ -328,7 +360,9 @@ def testGc_singleServer_gcAfterCacheMiss(self): # GC'd while server is alive: not cached, but also no crash self.sleepUntilCacheEmpty() _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) def testGc_multipleServers(self): module_bazel_lines = [ @@ -383,13 +417,195 @@ def testGc_multipleServers(self): ], cwd=dir_a, ) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) # GC'd while B's server is alive (after B's earlier cache hit): # not cached, but also no crash self.sleepUntilCacheEmpty() _, _, stderr = self.RunBazel(['build', '@my_repo//:haha'], cwd=dir_b) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) + + # TODO: Enable after RepositoryDelegatorFunction is moved to Skyframe + # workers, which is needed for batch checking to work correctly. + @unittest.skip('Requires Skyframe workers (not available on 8.7.0)') + def testReverseDependencyDirection(self): + # Set up two repos that retain their predeclared input hashes across two + # builds but still reverse their dependency direction. Depending on how + # repo cache candidates are checked, this could lead to a Skyframe cycle. + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(', + ' name = "foo",', + ' deps_file = "//:foo_deps.txt",', + ')', + 'repo(', + ' name = "bar",', + ' deps_file = "//:bar_deps.txt",', + ')', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' deps = rctx.read(rctx.attr.deps_file).splitlines()', + ' output = ""', + ' for dep in deps:', + ' if dep:', + ' output += "{}: {}\\n".format(dep, rctx.read(Label(dep)))', + ' rctx.file("output.txt", output)', + ' rctx.file("BUILD", "exports_files([\'output.txt\'])")', + ' print("JUST FETCHED: %s" % rctx.original_name)', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(', + ' implementation = _repo_impl,', + ' attrs = {', + ' "deps_file": attr.label(), }', + ')', + ], + ) + + self.ScratchFile('foo_deps.txt', ['@bar//:output.txt']) + self.ScratchFile('bar_deps.txt', ['']) + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@foo//:output.txt']) + self.assertIn('JUST FETCHED: bar', '\n'.join(stderr)) + self.assertIn('JUST FETCHED: foo', '\n'.join(stderr)) + + # After expunging and reversing the dependency direction: not cached + self.RunBazel(['clean', '--expunge']) + self.ScratchFile('foo_deps.txt', ['']) + self.ScratchFile('bar_deps.txt', ['@foo//:output.txt']) + self.RunBazel(['build', '@foo//:output.txt']) + + def doTestRepoContentsCacheDeleted(self, check_external_repository_files): + repo_contents_cache = self.ScratchDir('repo_contents_cache') + workspace = self.ScratchDir('workspace') + extra_args = [ + '--experimental_check_external_repository_files=%s' + % str(check_external_repository_files).lower(), + '--repo_contents_cache=%s' % repo_contents_cache, + ] + + self.ScratchFile( + 'workspace/MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile( + 'workspace/BUILD.bazel', + [ + 'genrule(', + ' name = "gen",', + ' srcs = ["@my_repo//:haha", "in.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $(SRCS) > $(OUTS)",', + ')', + ], + ) + self.ScratchFile( + 'workspace/repo.bzl', + [ + 'def _repo_impl(rctx):', + ( + ' rctx.file("BUILD", "filegroup(name=\'haha\',' + " srcs=['a.txt'], visibility=['//visibility:public'])\")" + ), + ' rctx.file("a.txt", "hello world")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + # First fetch: not cached + self.ScratchFile('workspace/in.txt', ['1']) + _, _, stderr = self.RunBazel( + [ + 'build', + '//:gen', + ] + + extra_args, + cwd=workspace, + ) self.assertIn('JUST FETCHED', '\n'.join(stderr)) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world1\n') + + # Second fetch: cached + self.ScratchFile('workspace/in.txt', ['2']) + _, _, stderr = self.RunBazel( + [ + 'build', + '//:gen', + ] + + extra_args, + cwd=workspace, + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world2\n') + + # Delete the entire repo contents cache and fetch again: not cached + # Avoid access denied on Windows due to files being read-only by moving to + # a different location instead. + os.rename(repo_contents_cache, repo_contents_cache + '_deleted') + self.ScratchFile('workspace/in.txt', ['3']) + _, _, stderr = self.RunBazel( + ['build', '//:gen'] + extra_args, + cwd=workspace, + ) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world3\n') + + # Second fetch after deletion: cached + self.ScratchFile('workspace/in.txt', ['4']) + _, _, stderr = self.RunBazel( + ['build', '//:gen'] + extra_args, + cwd=workspace, + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertNotIn('WARNING', '\n'.join(stderr)) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world4\n') + + # Delete the entire repo contents cache and fetch again with a different + # path: not cached + # Avoid access denied on Windows due to files being read-only by moving to + # a different location instead. + os.rename(repo_contents_cache, repo_contents_cache + '_deleted_again') + self.ScratchFile('workspace/in.txt', ['5']) + _, _, stderr = self.RunBazel( + ['build', '//:gen'] + + extra_args + + [ + '--repo_contents_cache=%s' % repo_contents_cache + '2', + ], + cwd=workspace, + ) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world5\n') + + def testRepoContentsCacheDeleted_withCheckExternalRepositoryFiles(self): + self.doTestRepoContentsCacheDeleted(check_external_repository_files=True) + + def testRepoContentsCacheDeleted_withoutCheckExternalRepositoryFiles(self): + self.doTestRepoContentsCacheDeleted(check_external_repository_files=False) if __name__ == '__main__': diff --git a/src/test/py/bazel/test_base.py b/src/test/py/bazel/test_base.py index 35f4c68c1ead07..eeee99fa3e41a1 100644 --- a/src/test/py/bazel/test_base.py +++ b/src/test/py/bazel/test_base.py @@ -402,7 +402,7 @@ def StartRemoteWorker(self): # worker path must be as short as possible so we don't exceed Windows # path length limits, so we run straight in TEMP. This should ideally # be set to something like C:\temp. On CI this is set to D:\temp. - worker_path = TestBase.GetEnv('TEMP') + worker_path = tempfile.mkdtemp(dir=TestBase.GetEnv('TEMP')) worker_exe = self.Rlocation('io_bazel/src/tools/remote/worker.exe') else: worker_path = tempfile.mkdtemp(dir=self._tests_root) @@ -464,6 +464,13 @@ def StopRemoteWorker(self): shutil.rmtree(self._cas_path) + def ClearRemoteCache(self): + """Clears the CAS of the "local remote worker".""" + self.assertIsNotNone(self._cas_path) + shutil.rmtree(self._cas_path) + # The worker needs the CAS path as well as the tmp dir to exist. + os.makedirs(os.path.join(self._cas_path, 'tmp')) + def RunProgram( self, args, diff --git a/src/test/shell/bazel/bazel_repository_cache_test.sh b/src/test/shell/bazel/bazel_repository_cache_test.sh index b698b9580420b5..c55799a73c3d17 100755 --- a/src/test/shell/bazel/bazel_repository_cache_test.sh +++ b/src/test/shell/bazel/bazel_repository_cache_test.sh @@ -541,4 +541,21 @@ EOF && fail "expected failure" || : } +function test_contents_cache_not_allowed_in_main_repo() { + # For some reason, this same test written in Python causes it to hang forever + # in the regression case, instead of failing immediately. So we write it in + # shell. + echo 'filegroup(name="foo")' > BUILD + if (bazel build --repo_contents_cache=inside foo >& $TEST_log); then + fail "expected exit code 2, got 0" + else + exit_code=$? + if [ $exit_code -ne 2 ]; then + fail "expected exit code 2, got $exit_code" + else + : + fi + fi +} + run_suite "repository cache tests" diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 4acba053b0d244..594fbdcf7cb3b3 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -419,7 +419,8 @@ private ActionResult execute( outErr, exitCode, startTime, - (int) wallTime.toMillis()); + (int) wallTime.toMillis(), + /* preserveExecutableBit= */ false); result = manifest.upload(context, cache, NullEventHandler.INSTANCE); } catch (ExecException e) { if (errStatus == null) {